-
-
Notifications
You must be signed in to change notification settings - Fork 9k
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-28379] Allow FingerprintFacet to block the deletion of Fingerprint #4478
[JENKINS-28379] Allow FingerprintFacet to block the deletion of Fingerprint #4478
Conversation
What is the current use case for this? Can you provide a downstream PR with an example for where you need it? |
@oleg-nenashev do you think this feature is still needed? thanks |
@MRamonLeon I am quoting below Oleg's response on Gitter.
|
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 am not sure you want to introduce a new field for fingerprintDeletionBlocked
at all. There is no API consumers at the moment. Maybe it is better to just implement an overridable isFingerprintDeletionBlocked()
which returns false by default.
P.S: Sorry for the late reviews, catching up
0a07375
to
25c3eb4
Compare
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 to me, thanks @stellargo !
987bc66
to
8815fbc
Compare
8815fbc
to
50cc8ce
Compare
@stellargo please, don't force-push on PR when it war already review by some people: it's hard to track 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.
I have no problem with the PR but asking few syntax improvements. Also would like to open the discussion about the return null
and the need for the annotation on the public API.
@alecharp Thanks for the review! I’ve made the requested changes. Let me know if anything more is required. |
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.
Thanks for addressing the comments. I believe we can merge it, just a single comment about the naming
Thanks @oleg-nenashev , I have made the requested changes. |
a00e466
to
7ace7a7
Compare
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 plan to merge it tomorrow if no negative feedback
ATH is stuck due to infra issues. I will ignore it and merge the PR. It was passing before |
Thanks for your contribution @stellargo ! |
Thanks @oleg-nenashev and everyone who guided me through it. I learnt a lot :) |
See JENKINS-28379.
Proposed changelog entries
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