Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[JENKINS-22126] override build parameters only when explicitly requested #26

Merged
merged 1 commit into from May 28, 2014
Merged

[JENKINS-22126] override build parameters only when explicitly requested #26

merged 1 commit into from May 28, 2014

Conversation

ndeloof
Copy link
Contributor

@ndeloof ndeloof commented Mar 11, 2014

add a configuration option to explicitly ask envinject to override build parameters. Don't override by default and don't override when envinject isn't used on jobs.

oleg-nenashev
oleg-nenashev reviewed Mar 11, 2014
View reviewed changes
src/main/java/org/jenkinsci/plugins/envinject/EnvInjectJobProperty.java
@@ -164,6 +173,7 @@ public EnvInjectJobProperty newInstance(StaplerRequest req, JSONObject formData)
JSONObject onJSONObject = (JSONObject) onObject;
envInjectJobProperty.setKeepJenkinsSystemVariables(onJSONObject.getBoolean("keepJenkinsSystemVariables"));
envInjectJobProperty.setKeepBuildVariables(onJSONObject.getBoolean("keepBuildVariables"));
envInjectJobProperty.setKeepBuildVariables(onJSONObject.getBoolean("overrideBuildParameters"));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like a typo. I suppose you should call setOverrideBuildParameters

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

indeed

@cloudbees-pull-request-builder
Copy link

plugins » envinject-plugin #43 UNSTABLE
Looks like there's a problem with this pull request

@oleg-nenashev
Copy link
Member

From my side, it makes sense to leave the legacy behavior by default.
Otherwise, it may cause regression in user jobs.

@ndeloof
Copy link
Contributor Author

ndeloof commented Mar 11, 2014

envinject "legacy" behavior is buggy : it applies even user didn't configure anything on job, it overrides parameters that is contra-intuitive. I would expect a dozen of users to rely on this

@cloudbees-pull-request-builder
Copy link

plugins » envinject-plugin #44 UNSTABLE
Looks like there's a problem with this pull request

@jenkinsadmin
Copy link
Member

Thank you for a pull request! Please check this document for how the Jenkins project handles pull requests

@oleg-nenashev
Copy link
Member

@ndeloof
You're right, the current behavior is not good.
I see that the current behavior is not documented, so it would be enough to explicitly mention possible migration issues in the changelist.

gboissinot added a commit that referenced this pull request May 28, 2014
@gboissinot
Merge pull request #26 from ndeloof/JENKINS-22126
91cac44 
[JENKINS-22126] override build parameters only when explicitly requested
@gboissinot gboissinot merged commit 91cac44 into jenkinsci:master May 28, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
5 participants
@ndeloof @cloudbees-pull-request-builder @oleg-nenashev @jenkinsadmin @gboissinot