Add 'reason' field in the lock() step#520
Add 'reason' field in the lock() step#520Massakera wants to merge 11 commits intojenkinsci:masterfrom
Conversation
|
Issue that I'm still trying to solve (Labels shifting to the Reason column in the Lockable Resources page): #426 (comment) |
you need to add the |
It worked! I have also added a couple of things inside |
mPokornyETM
added
enhancement
java
|
|
||
| @DataBoundSetter | ||
| public void setReason(String reason) { | ||
| if (reason != null && !reason.isEmpty()) { |
There was a problem hiding this comment.
Should there be a way to remove a "reason" (e.g. in current code setting it to null or an empty string won't work - is that intentional)?
In other words, once a reason is saved with a lockable resource, is it expected that it can only be replaced by another non-trivial reason value?
There was a problem hiding this comment.
Hmm I thought that this would make sense, I mean if you're locking something you should have a reason to do that. What you think?
There was a problem hiding this comment.
Arguably maybe, but by "principle of least surprise" they should not be suddenly required to do so (see also the other question about possibly new required parameter and so rewrite of existing pipelines).
By the way, while at it (not at PC now to check easily): does this PR also update the unlock/unreserve/recycle etc. logics to clear the reason when forgetting the LR was locked? If it were to do so by calling setReason(null), well...
| public int quantity = 0; | ||
|
|
||
| LockStepResource(@Nullable String resource, @Nullable String label, int quantity) { | ||
| LockStepResource(@Nullable String resource, @Nullable String label, int quantity, @Nullable String reason) { |
There was a problem hiding this comment.
Related to the question on removing/nulling an existing reason -- do we intend to require that a non-trivial one gets set by constructor (possibly including loader of older configs)?
For backwards compatibility, I'd add a method with an old signature which calls the new one with a null argument. For constructors I guess one has to implement several copies completely. General pattern:
SomeMethod(old, args, NewArg) { ... }
SomeMethod(old, args) { return SomeMethod(old, args, null); }
There was a problem hiding this comment.
In other words, do I understand it correctly that here the lock(...) {} step will require a new argument, so requiring all existing pipelines to be rewritten to add it?.. That would be counterproductive (having to add resource : null is annoying enough already...)
| } | ||
|
|
||
| public static String toString(String resource, String label, int quantity) { | ||
| public static String toString(String resource, String label, int quantity, String reason) { |
There was a problem hiding this comment.
Is reason intentionally ignored here? Or should it be suffixed (if not null/empty) to other replies?
|
@Massakera can you merge it pls? Massakera#1 I will continue here, also when you has no more time. Thx |
Sure! Thanks for the help! |
mPokornyETM
added
the
got conflicts
3 participants
resolves #426
Testing done
Reason field in the settings page, when adding a Lockable Resource:
Reason field viewed from the Lockable resource page:
Proposed upgrade guidelines
N/A
Localizations
Submitter checklist
@NoExternalUse. In case it is used by non java code theUsed by {@code <panel>.jelly}Javadocs are annotated.evalto ease the future introduction of Content Security Policy (CSP) directives (see documentation).Maintainer checklist
Before the changes are marked as
ready-for-merge:upgrade-guide-neededlabel is set and there is a Proposed upgrade guidelines section in the pull request title (see example).