-
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
allow file to be null for use in CasC #11
Conversation
just |
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.
Seems a bit suspect but the databinding here was already rather suspect.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
file
to 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