-
Notifications
You must be signed in to change notification settings - Fork 35
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
FEAT: allow for null file for CasC support. #13
FEAT: allow for null file for CasC support. #13
Conversation
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.
Sounds reasonable to me!
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.
<dependency> | ||
<groupId>io.jenkins.configuration-as-code</groupId> | ||
<artifactId>configuration-as-code-support</artifactId> | ||
<version>1.3</version> |
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.
Desirable to introduce a POM property to capture the commonality.
@@ -78,6 +79,7 @@ | |||
*/ | |||
@Nonnull | |||
private final SecretBytes secretBytes; | |||
|
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.
could be reverted
@@ -34,6 +34,7 @@ | |||
import java.io.InvalidObjectException; | |||
import java.io.ObjectStreamException; | |||
import java.security.GeneralSecurityException; | |||
import java.util.Optional; |
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.
BTW I think this makes the code harder to read than plain old null checks.
.orElse(bytes); | ||
} | ||
|
||
private SecretBytes resolveSecretBytes(FileItem file, String data) { |
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 both overloads are only called once, could they just be inlined back to where they were? It is hard to scroll back and forth to see what these methods are doing.
@@ -159,7 +150,7 @@ public FileCredentialsImpl(@CheckForNull CredentialsScope scope, @CheckForNull S | |||
} | |||
if (LOGGER.isLoggable(Level.FINE)) { | |||
LOGGER.log(Level.FINE, "for {0} have {1} of length {2} after upload of ‘{3}’", | |||
new Object[]{getId(), this.fileName, this.secretBytes.getPlainData().length, name}); | |||
new Object[]{getId(), this.fileName, this.secretBytes.getPlainData().length, file.getName()}); |
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.
Will this not throw NPE when file == null
, as according to the constructor annotation it is permitted to?
Thanks but I have prepared a more minimal patch. |
I understand @jglick I am not used to working on forked repositories and did not know how to go about the non-destructive approach. Therefore decided to just go with my own solution, and while at it clean up the code a bit. Hope your solution gets merged soon! Thank you. |
Released in 1.5. Thank you again for driving this forward! |
@jglick This is follow up to #11.