-
Notifications
You must be signed in to change notification settings - Fork 30
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
[JENKINS-73769] account for empty libraries or changed library path #137
[JENKINS-73769] account for empty libraries or changed library path #137
Conversation
2dee7b2
to
687ca30
Compare
687ca30
to
379a825
Compare
ef79515
to
b2a3113
Compare
src/main/java/org/jenkinsci/plugins/workflow/libs/LibraryRecord.java
Outdated
Show resolved
Hide resolved
if (retriever instanceof SCMBasedRetriever) { | ||
libraryPath = ((SCMBasedRetriever) retriever).getLibraryPath(); | ||
} | ||
librariesAdded.put(name, new LibraryRecord(name, version, kindTrusted, changelog, cfg.getCachingConfiguration(), source, libraryPath)); |
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.
I don't remember exactly how all the retrieval works, so I am wondering if this could potentially cause unnecessary retrieval to happen? E.g. One git repository containing five libraries would only be retrieved one time for each scm url ignoring libraryPath, and now with this change it will retrieve/cache one copy for each libraryPath? And that might not be desired?
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.
Not as far as I can tell. The (SCM based) libraries are checked out:
- in the jobs own workspace
- only if needed (i.e. either caching not enabled, expired, or something else changed)
bash-4.4$ find -type d -name ".git"
./workspace/test-pl3@libs/cd304ec6497d6f157eab60a432dcc6ecc1350d5f15d1fae9474b01115c211903/.git
./workspace/test-pl2@libs/cd304ec6497d6f157eab60a432dcc6ecc1350d5f15d1fae9474b01115c211903/.git
./workspace/test-pl@libs/cd304ec6497d6f157eab60a432dcc6ecc1350d5f15d1fae9474b01115c211903/.git
Library caching happens after checkout and the files land in a different directory (based on directoryName
) and without the .git
extras. Below two cached directories representing the vars under:
- https://github.com/sboardwell/demos/tree/main (root)
- https://github.com/sboardwell/demos/tree/main/build-storm (libraryPath = build-storm)
bash-4.4$ find global-libraries-cache/ -type f -newermt $(date +%Y-%m-%d) | sort -k 8
global-libraries-cache/2079b09ce5b9d359d406cb4c826fc11b39a8bb5c01398d55689261d81ebe1a11-name.txt
global-libraries-cache/2079b09ce5b9d359d406cb4c826fc11b39a8bb5c01398d55689261d81ebe1a11/last_read
global-libraries-cache/2079b09ce5b9d359d406cb4c826fc11b39a8bb5c01398d55689261d81ebe1a11/vars/deleteLibsDir.groovy
global-libraries-cache/2079b09ce5b9d359d406cb4c826fc11b39a8bb5c01398d55689261d81ebe1a11/vars/echoMe.groovy
global-libraries-cache/2079b09ce5b9d359d406cb4c826fc11b39a8bb5c01398d55689261d81ebe1a11/vars/rebaseRepos.groovy
global-libraries-cache/5a7d69d45c180afdf21fe343b63c3c3d4b7988c0dbc0ac83ff1d0977f7aba33f-name.txt
global-libraries-cache/5a7d69d45c180afdf21fe343b63c3c3d4b7988c0dbc0ac83ff1d0977f7aba33f/last_read
global-libraries-cache/5a7d69d45c180afdf21fe343b63c3c3d4b7988c0dbc0ac83ff1d0977f7aba33f/src/lostinberlin/TestClass.groovy
global-libraries-cache/5a7d69d45c180afdf21fe343b63c3c3d4b7988c0dbc0ac83ff1d0977f7aba33f/src/lostinberlin/TestEnum.groovy
global-libraries-cache/5a7d69d45c180afdf21fe343b63c3c3d4b7988c0dbc0ac83ff1d0977f7aba33f/vars/echoMe.groovy
global-libraries-cache/5a7d69d45c180afdf21fe343b63c3c3d4b7988c0dbc0ac83ff1d0977f7aba33f/vars/myPipeline.groovy
global-libraries-cache/5a7d69d45c180afdf21fe343b63c3c3d4b7988c0dbc0ac83ff1d0977f7aba33f/vars/rebuildOnDisconnect.groovy
global-libraries-cache/5a7d69d45c180afdf21fe343b63c3c3d4b7988c0dbc0ac83ff1d0977f7aba33f/vars/retryOnDisconnect.groovy
global-libraries-cache/5a7d69d45c180afdf21fe343b63c3c3d4b7988c0dbc0ac83ff1d0977f7aba33f/vars/updateBuildSchedule.groovy
I am not maintaining this plugin, especially not anything to do with caching which I do not think should have been introduced to begin with. |
I am still hoping for one more set of eyes to review this. Maybe @car-roll ? |
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.
LGTM
FTR I am not a maintainer of that plugin and have very limited knowledge about it. I have read more code from that plugin to better understand how it works and played with the test and reproducer you provided to better understand your contribution and it looks great to me.
Thanks a lot @sboardwell for all the details in there!
This pull request:
EMPTY
status to the CacheStatusLibraryAdder
libraryPath
to the caching directory namelibraryPath
on the shared library configuration had changedSteps to reproduce
libraryPath not considered
Consider a repo with multiple libraries
Empty library on checkout
vars
orsrc
directoriesEmpty library do to something unexpected
Context
Although using the version which should have fixed the issue, some jobs have shown the following:
Testing done
Tests added to PR.
Submitter checklist