-
Notifications
You must be signed in to change notification settings - Fork 30
Allow specific version to be deleted from cache #4
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
@jglick I have moved my pull request over to this repository for review as requested. |
jenkinsci/workflow-cps-global-lib-plugin#142 for the record. |
I'm working with this plugin the same as workflow-cps-global-lib-plugin but all the tests locally fail with an IllegalStateException on this line. Am I missing something new with this project setup? |
If an mvn -Pquick-build clean install (Ultimately probably a bug in either the annotation processor or the Maven compiler plugin or somehow the interaction between the two, I just have never tried to track it down.) |
Cool. That did the trick. After running your command the tests are working again. Seems like it's probably some specific eclipse m2e interaction that's failing. I'll investigate those failing tests. They probably don't like how I restored the nested cache directory structure. |
0d00e64
to
e384a3f
Compare
Unfortunately I only have a windows machine to test on and I can't run that symlink test locally. I looked over the logic that it's calling and I don't have much of an explanation as to why that test fails. LibraryRecord.getDirectoryName() returns the new nested path and that's used to compare LibraryAdder.findResources. I'll load my fork on my linux build server later today and try to debug further. |
defd8cd
to
5444f8b
Compare
The getConnicalFile method does not appear to be returning the symlink path as expected. 2.810 [id=51] SEVERE o.j.p.workflow.libs.LibraryAdder#findResources: path: /home/jenkins/workspace/_pipeline-groovy-lib-plugin_PR-4/target/tmp/j h1113867621410959948/jobs/p/builds/1/libs/5fbd776874e55a452cf050b3df731fe13a36dbcebf52977cb67b1fef6615d8f3/d130612e7c729d275f2281426a605830ebce666d4089ff4ec5ded2322d6e773d/resources/master.key I will continue to investigate. |
5444f8b
to
335fe2d
Compare
@jglick I think the root cause of the failing test is that getCanonicalFile only works on symlinks that actually exist. secrets/master.key an actual file (whoops). When I updated the library directory path to include an additional folder I broke the symlink. I will clean up my commits and then this pull request will be ready for review. Is there a style guide for ordering imports? I see that eclipse rearranged them. I'll reorder them. |
To be clear, I am not claiming to maintain this plugin generally (I needed to do the split from |
Who should I loop in to the pr via comments? It seems like the default permissions don't allow me to add reviewers. |
ada6bad
to
877434b
Compare
if (libraryCachePath != null) { | ||
FilePath versionCachePath = new FilePath(libraryCachePath, libraryNamePath.getName().replace("-name.txt", "")); | ||
if (cachedLibraryRef != null && !cachedLibraryRef.equals("")) { | ||
if (libraryNamePath.readToString().equals(name + "@" + cachedLibraryRef)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use StringUtils.isNotBlank(cachedLibraryRef) to also catch strings containing only whitespace
} | ||
} else { | ||
cacheDirName = name; | ||
libraryCachePath.deleteRecursive(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When #3 is merged you will not be able to delete like this. You must delete each version separately to ensure it is not just getting used by any other operation (e.g. cache was expired and is getting refreshed, a build is just copying it).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure. I will integrate your changes depending on which gets merged first.
877434b
to
4f59a0b
Compare
4f59a0b
to
f261a36
Compare
@mawinter69 as somewhat of an initial compromise with simplicity in mind I have chosen to lock the entire library folder while performing read or write actions. This will force a wait for library@version1 and library@version2 because the lock being acquired is "library". I am not opposed to individually locking each sub directory however cleanup of the entire set of caches becomes much more complicated. |
@@ -202,7 +202,7 @@ static List<URL> retrieve(@NonNull LibraryRecord record, @NonNull LibraryRetriev | |||
FilePath libDir = new FilePath(execution.getOwner().getRootDir()).child("libs/" + record.getDirectoryName()); | |||
Boolean shouldCache = cachingConfiguration != null; | |||
final FilePath versionCacheDir = new FilePath(LibraryCachingConfiguration.getGlobalLibrariesCacheDir(), record.getDirectoryName()); | |||
ReentrantReadWriteLock retrieveLock = getReadWriteLockFor(record.getDirectoryName()); | |||
ReentrantReadWriteLock retrieveLock = getReadWriteLockFor(record.getName()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is to make it consistent across accesses because record.getName() is no longer equal to record.getDirectoryName().
1f44c2d
to
cee52c0
Compare
You can now specify a specific ref to be removed from the cache rather than having to remove all of them
cee52c0
to
a9f1645
Compare
@jglick @car-roll @mawinter69 Any feedback on this pull request? |
You can now specify a specific ref to be removed from the cache
rather than having to remove all of them
Restores the nested cache structure from before SECURITY-2586.