allow file to be null for use in CasC #11
Conversation
just |
|
also merge w/ #10 to get CI |
|
test failure on windows, which also happens on master : https://ci.jenkins.io/job/Plugins/job/plain-credentials-plugin/job/master/1/testReport/org.jenkinsci.plugins.plaincredentials/SecretBytesTest/windows___Archive__windows____migrateLegacyData/ |
|
When to fix this test failure and release a new version ? ^_^ |
|
@ndeloof merge with |
Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
|
rebased on origin/master + test-case added to demonstrate casc-support and avoid regression in future changes to this plugin |
|
|
Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
|
|
||
| @Test | ||
| public void should_configure_file_credentials() throws Exception { | ||
| SecretBytes.decrypt(null); // force class loading to get Stapler converter registered |
There was a problem hiding this comment.
This does not look right. You are referring to SecretBytes.StaplerConverterImpl? I am not sure what normally causes it to be registered, but if it is not being registered implicitly by the rest of the test case, something is broken and that needs to be fixed first.
There was a problem hiding this comment.
I have no idea why it doesn't get loaded, static initializer isn't executed as demonstrated running with a debugger. No idea what could go wrong here.
There was a problem hiding this comment.
According to this code, the converter is used by virtue of its class name. That would mean that the abovementioned line could be deleted and the test would still pass.
There was a problem hiding this comment.
And jenkinsci/configuration-as-code-plugin#317 does not have such a trick, so…?
|
closing as tired having to fight with jenkins weirdnesses |
|
Can you just check the |
|
@jglick I am going to take over this work from @ndeloof as mentioned here: jenkinsci/configuration-as-code-plugin#271. Can you close this in favor of the PR I will open later today? It will contain these changes, and make sure we don't need |
Thanks!
No need. So long as you avoid destructive history operations, GitHub is smart enough to mark a PR as merged as soon as its head reference becomes an ancestor of the base branch—including when a subsuming PR is merged. |
Too late for that; #13 seems to have squashed everything and is thus misattributing authorship. |
|
Never mind; #13 is much harder to review and not as obviously safe. I will pick this up. |
requirement for
fileto be non-null just to get a name prevents simple use of FileCredentialsImpl in Configuration-as-Code.deployed as plain-credentials-1.5-20180703.101522-1
See jenkinsci/configuration-as-code-plugin#317