-
Notifications
You must be signed in to change notification settings - Fork 107
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-72611] Forbid updating credentials ID #366
Conversation
complementing jenkinsci/credentials-plugin#502 |
most likely needs to be reverted due to exactly the same reason as jenkinsci/credentials-plugin#502 |
// Note: We probably should not assume that all credentials are IdCredentials. We also do this a few lines below in list.indexOf(current) call. | ||
// If this one breaks, that one breaks as well. | ||
if (!current.equals(replacement)) { | ||
throw new IllegalArgumentException("Credentials' IDs do not match, will not update."); |
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.
no no and more no. you must handle credentials that are not IDCredentials
reverted in #368
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.
What about int index = list.indexOf(current)
6 lines below though? It will rely on equals
under the hood, won't 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.
Yes it does.
But why do you think that matters?
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.
If credential isn't an IdCredential
, would that indexOf
correctly find a credential to be updated?
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.
If credential isn't an IdCredential, would that indexOf correctly find a credential to be updated?
Under what conditions would it fail to find current
in a list of Credentials
?
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.
Object.equals
.
I've tested locally with the loadfocus plugin, and updating indeed works there. I'm surprised that object reference is that same. I don't know why I assumed it wouldn't be.
See JENKINS-72611.
Proposed changelog entries
Submitter checklist
* Use the
Internal:
prefix if the change has no user-visible impact (API, test frameworks, etc.)