-
-
Notifications
You must be signed in to change notification settings - Fork 8.8k
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-62345] Introduce FingerprintStorage API in core #4731
[JENKINS-62345] Introduce FingerprintStorage API in core #4731
Conversation
This draft PR is what I will be working on for changes on the core side. I have made some basic adjustments as a start for moving the saving functionality to FileFingerprintStorage. |
c778655
to
6b873a4
Compare
Apologies for the force push, I really messed up the git history, so had to do it :( |
Shouldn't new files be better placed in the Jenkins namespace instead of hudson? I did not check if this has implications. |
1e3c1f4
to
a98412f
Compare
@oleg-nenashev Thanks! Incorporated the changes :) |
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.
From what I see, it is ready to go. Thanks a lot for the hard work @stellargo !
Before we merge it, we need to prepare changelog entries for the pull request. See the Pull request template for examples. I would suggest two entries:
- Developer API change for the new Fingerprint API and deprecating old File ones (CRUD, retrieving persisted facets)
- New Experimental External Fingerprint Storage API (with a link to JEP and a reference implementation)
Credentials plugin ran successfully without any failing tests on this PR's incremental build https://ci.jenkins.io/blue/organizations/jenkins/Plugins%2Fcredentials-plugin/detail/PR-153/1/pipeline |
@oleg-nenashev Thank you for reviewing, I have added the suggested changelog entries. Kindly let me know if any more changes are required :) |
We may merge it in 24 hours if there is no negative feedback. Please see the merge process documentation for more information about the merge process |
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.
looks good,
I haven't ran it interactively.
no blockers, if you have time there's some minor improvements that could be made to docs and code style.
core/src/main/java/jenkins/fingerprints/FileFingerprintStorage.java
Outdated
Show resolved
Hide resolved
core/src/main/java/jenkins/fingerprints/FileFingerprintStorage.java
Outdated
Show resolved
Hide resolved
core/src/main/java/jenkins/fingerprints/FingerprintStorage.java
Outdated
Show resolved
Hide resolved
/** | ||
* Pluggable fingerprint storage API for fingerprints. | ||
* | ||
* @author Sumit Sarin |
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 entire class needs an @since TODO
, same with FileFingerprintStorage
.
* The file we save our configuration. | ||
* Loads a {@link Fingerprint} from the Storage with the given unique id. | ||
* @return Loaded {@link Fingerprint}. {@code null} if the config file does not exist or | ||
* malformed. |
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.
@since
?
See JENKINS-62345.
See JEP-226
Proposed changelog entries
Fingerprint#getPersistedFacets()
,Fingerprint#delete(String)
. Deprecated methods:Fingerprint#save(File)
. This API expects usage of standard CRUD operations likeFingerprint#save()
,Fingerprint#load(String)
andFingerprint#delete(String)
Proposed upgrade guidelines
N/A
Submitter checklist
Proposed changelog entries
section only if there are breaking changes or other changes which may require extra steps from users during the upgradeDesired reviewers
@mention
Maintainer checklist
Before the changes are marked as
ready-for-merge
:Proposed changelog entries
are correctupgrade-guide-needed
label is set and there is aProposed upgrade guidelines
section in the PR title. (example)lts-candidate
to be considered (see query).