[JENKINS-26583] - Do not suppress environment variables contributed by extensions #125
Conversation
Integrates tests only, the implementation is reverted since the fix does not work anyway # Conflicts: # src/main/java/org/jenkinsci/plugins/envinject/EnvInjectListener.java # src/test/java/org/jenkinsci/plugins/envinject/BuildCauseRetrieverTest.java
…ted to JENKINS-26583
|
This pull request originates from a CloudBees employee. At CloudBees, we require that all pull requests be reviewed by other CloudBees employees before we seek to have the change accepted. If you want to learn more about our process please see this explanation. |
|
I verified that this PR fixes https://issues.jenkins-ci.org/browse/JENKINS-26583 . Please merge asap! |
|
Retriggering CI |
| for (Map.Entry<String, String> storedVar : currentEnvMap.entrySet()) { | ||
| final String varName = storedVar.getKey(); | ||
| final String storedValue = storedVar.getValue(); | ||
| final String envValue = env.get(storedVar.getKey()); |
There was a problem hiding this comment.
I suggest to use local variable varName defined 2 row above
| } | ||
|
|
||
| if (usedExternalValue) { // The value was overridden, let's update the cache | ||
| LOGGER.log(Level.CONFIG, "Build {0}: Variable {1} is defined externally, overriding the stored value {2} by {3}", |
There was a problem hiding this comment.
Are password logged in plain text?
| boolean usedExternalValue = true; | ||
| if (parameterEnvVars != null) { | ||
| String parameterValue = parameterEnvVars.get(varName); | ||
| if (envValue.equals(parameterValue)) { // defined by parameter and not already overridden |
There was a problem hiding this comment.
In the previous comment you refer to shouldOverrideBuildParametersIfEnabled() settings.
I do not see where the value is taken, I would expect that value in && here
There was a problem hiding this comment.
shouldOverrideBuildParametersIfEnabled() is a test, there is no such configuration option
There was a problem hiding this comment.
I remember that option in the job configure page:
Should this option be ignored in override policy?
There was a problem hiding this comment.
agreed, needs investigation
There was a problem hiding this comment.
I agree with @nfalco79 here, I think since the existing behavior is being changed anyway, it makes sense to start checking that option here. From what I can see, that option is currently only used in the EnvInjectBuildVariableContributor class which does something similar to this piece of code. Tests pass for me locally with the change:
EnvInjectJobProperty envInjectJobProperty = RunHelper.getEnvInjectJobProperty(build);
if (parameterEnvVars != null && envInjectJobProperty != null && envInjectJobProperty.isOverrideBuildParameters() && parameterEnvVars != null) {
String parameterValue = parameterEnvVars.get(varName);
|
@nfalco79 Thanks for catching it! Updated the PR |
|
Also for me seems work (PATH env var is valid) |
|
I still anticipate some regressions, likely I will add a compatibility warning. In #124 I tried to do more things about the resolution order (e.g. Extension listing order), but it is a subject for the next pull requests. |
|
Ok, will do it and add extra test
…On Oct 3, 2017 8:38 PM, "Devin Nusbaum" ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In src/main/java/org/jenkinsci/plugins/envinject/
EnvInjectPluginAction.java
<#125 (comment)>
:
> + for (Map.Entry<String, String> storedVar : currentEnvMap.entrySet()) {
+ final String varName = storedVar.getKey();
+ final String storedValue = storedVar.getValue();
+ final String envValue = env.get(storedVar.getKey());
+ if (envValue == null) {
+ LOGGER.log(Level.CONFIG, "Build {0}: Variable {1} is missing, overriding it by the stored value {2}",
+ new Object[] {build, varName, storedValue});
+ env.put(varName, storedValue);
+ } else if (!envValue.equals(storedValue)) {
+ // If the value is defined by the Parameters, we actually override them
+ // See org.jenkinsci.plugins.envinject.EnvInjectJobPropertyTest#shouldOverrideBuildParametersIfEnabled()
+ final EnvVars parameterEnvVars = getParameterEnvVars();
+ boolean usedExternalValue = true;
+ if (parameterEnvVars != null) {
+ String parameterValue = parameterEnvVars.get(varName);
+ if (envValue.equals(parameterValue)) { // defined by parameter and not already overridden
I agree with @nfalco79 <https://github.com/nfalco79> here, I think since
the existing behavior is being changed anyway, it makes sense to start
checking that option here. From what I can see, that option is currently
only used in the EnvInjectBuildVariableContributor
<https://github.com/jenkinsci/envinject-plugin/blob/127850b92b01a0430b1edc7ce428e7d55626af6b/src/main/java/org/jenkinsci/plugins/envinject/EnvInjectBuildVariableContributor.java#L34>
class which does something similar to this piece of code. Tests pass for me
locally with the change:
EnvInjectJobProperty envInjectJobProperty = RunHelper.getEnvInjectJobProperty(build);
if (parameterEnvVars != null && envInjectJobProperty != null && envInjectJobProperty.isOverrideBuildParameters() && parameterEnvVars != null) {
String parameterValue = parameterEnvVars.get(varName);
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#125 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AKBqVI5Up18QAxy6DMfmVL8TuRxaLhZAks5son8lgaJpZM4PoELU>
.
--
You received this message because you are subscribed to the Google Groups
"engineering-code-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an
email to ***@***.***
To post to this group, send email to engineering-code-reviews@
cloudbees.com.
To view this discussion on the web visit https://groups.google.com/a/
cloudbees.com/d/msgid/engineering-code-reviews/jenkinsci/envinject-plugin/
pull/125/review/66855463%40github.com
<https://groups.google.com/a/cloudbees.com/d/msgid/engineering-code-reviews/jenkinsci/envinject-plugin/pull/125/review/66855463%40github.com?utm_medium=email&utm_source=footer>
.
|
…y before overriding build parameters
|
@dwnusbaum @nfalco79 done |
|
My plan is to release it today/tomorrow together with jenkinsci/envinject-lib#11 from @dwnusbaum |
|
@reviewbybees done |
|
Merging without squash since there are multiple contributors. No plans to backport this tricks anyway |
|
The fix caused regressions (no wonder), I am blacklisting the release for a while to investigate them: jenkins-infra/update-center2#167 . Likely I will respin the release and disable the new behavior by default (and add a flag in UI) |
|
@oleg-nenashev I reproduced the reported issues and agree with blacklisting it for now. Hiding the new functionality behind a checkbox that says something like "Do not override environment variables defined elsewhere" makes sense to me, and I think we should also add a few tests to prevent a regression in the future. I would like to know if the people who were blocked by JENKINS-26583 find that 2.1.4 works for them correctly or has any issues. |
Checkbox solution will not work because the issue JENKINS-26583 happens just installing this plugin. Actually I had install 2.1.4 in our system and seems the PATH variable is ok because the nodejs command is available. Anyway let me do other try |
|
Just to be informed, what is the regression? |
|
@nfalco79 JENKINS-47364 and JENKINS-47370, users relied on environment variables defined elsewhere being overridden by the plugin, which is exactly the functionality that this PR removed. |
|
JENKINS-47364 seems a real regression where variables are not substituted. Instead in JENKINS-47370 if I interpreted right the option "Override Build Parameters" he miss to enable it (taking care about only the use case in the issue description, not the following comments). |
|
For JENKINS-47370 I tested with that setting but it was still broken. From what I can tell the user just has it a set as a environment variable in Jenkins' global properties configuration, and the "Override Build Parameters" setting only deals with parameters, not global environment variables. |
|
@nfalco79 In any case, I am not going to revert the fix. My plan is to temporary disable it while I am figuring out what to do with regressions in non-Pipeline use-cases. In the case of Pipeline, we can break whatever we want in the current state (there is a warning in Wiki). Feature flag is something under consideration. If the impact is not high, I also consider keeping the new behavior by default. |
It is a partial version of #124, which does not fix all tests created by @olivergondza. But it at least resolves the issue reported in JENKINS-26583, at least on the common paths.
EnvInjectPluginActionfrom overriding variables on late stages, 3 tests fixedEnvInjectListenerto call it first and to let others override the EnvVarsEnvInjectPluginActionstill overrides build parameters, just because it is covered by testsMore fixes are to come later.
@reviewbybees @olivergondza @recena