FEAT: allow for null file for CasC support. #13
Conversation
| <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.
Desirable to introduce a POM property to capture the commonality.
| @@ -78,6 +79,7 @@ | |||
| */ | |||
| @Nonnull | |||
| private final SecretBytes secretBytes; | |||
|
|
|||
| @@ -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.
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.
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.
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.