Allow empty valued variables without ignoring enviroment vars substitution#2665
Allow empty valued variables without ignoring enviroment vars substitution#2665timja merged 12 commits intojenkinsci:masterfrom
Conversation
…onfiguration-as-code-plugin into empty-vars-exporting
| value: BAR | ||
| - key: FOO2 | ||
| value: "" | ||
| - key: FOO3 |
There was a problem hiding this comment.
This (FOO3) just looks like a user error to me ⇒ the bundle should be rejected.
There was a problem hiding this comment.
UI allows to save an empty env var (regardless why would someone want that), so theoretically configuration as code should allow creating that configuration too.
There was a problem hiding this comment.
I am talking about FOO3 (null) not FOO2 (empty).
There was a problem hiding this comment.
That's the point in allowing importing the exported yaml.
Empty vars will still be exported with no value, so it's either allowing keys without a value when importing or changing the export (which was discussed here).
I think we all agree that would be the optimal solution, but when testing that it impacted exports from several plugins, so that's not what we're trying to cover with this change
There was a problem hiding this comment.
the point is that
- key: FOO
(no value)is invalid and should not be allowed (unless it works right now and this is just testing something pre-existing)
a null env var value is invalid.
an empty string env var uncommon, but I guess
so ok:
- key: FOO
value: ""not ok:
- key: FOOThere was a problem hiding this comment.
Can we not just limit the behavioral change to this setting, leaving other misbehaving plugins for another day?
There was a problem hiding this comment.
Looks do-able from a quick look, something similar to this on CNode and then allow setting the value when you construct it.
Not sure how easy it would be to integrate when using databoundconfigurator though.
There was a problem hiding this comment.
I'm trying that, with no luck for the moment
There was a problem hiding this comment.
Updated be00ec1:
- Removed the changes when configuring, so importing null vars is not supported
- Added a false by default flag to CNode to allow printing when empty and setting it to trtue only for non null env vars
Will update PR description accordingly
|
Related test failing on Java 21 |
Forgot to remove test condition from previous implementation 😅 Fixed now |
5 participants
fixes: #2658
Changes export behaviour for global environment variables. Now empty env vars will be exported as empty values instead of key without value as they were until now.
No change when configuring, behaviour is the same as before this PR.
Additinally, added a test covering varlues with environment variable resolution.
Your checklist for this pull request
🚨 Please review the guidelines for contributing to this repository.