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.

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

[JENKINS-26100] Change buildEnvVars to take Run #102

Merged
merged 5 commits into from May 19, 2017

Conversation

Projects
None yet
4 participants
@abayer
Copy link
Member

commented May 1, 2017

JENKINS-26100

FYI, I had some weird test failures locally when I ran mvn clean install but couldn't reproduce when running, say, mvn clean install -Dtest=PipelineTest so I'm not sure if they're environmental or what, so...hey, why not, here's a PR.

cc @reviewbybees

@abayer abayer requested a review from jglick May 1, 2017

@jglick

jglick approved these changes May 1, 2017

Copy link
Member

left a comment

OK except for requested buildEnvVars rename.

pom.xml Outdated
<no-test-jar>false</no-test-jar>
<scm-api-plugin.version>2.0.8</scm-api-plugin.version>
<scm-api-plugin.version>2.1.0</scm-api-plugin.version>

This comment has been minimized.

Copy link
@jglick

jglick May 1, 2017

Member

java.level → 8

pom.xml Outdated
@@ -14,9 +14,10 @@
<url>http://wiki.jenkins-ci.org/display/JENKINS/Mercurial+Plugin</url>

<properties>
<jenkins.version>1.642.3</jenkins.version>
<jenkins-core.version>2.58-20170502.192524-8</jenkins-core.version> <!-- TODO: Switch to release once https://github.com/jenkinsci/jenkins/pull/2730 is merged and released -->
<jenkins-war.version>2.58-20170502.192544-8</jenkins-war.version> <!-- TODO: Switch to release once https://github.com/jenkinsci/jenkins/pull/2730 is merged and released -->

This comment has been minimized.

Copy link
@jglick

This comment has been minimized.

Copy link
@jglick

jglick May 16, 2017

Member

2.60 I meant.

@reviewbybees

This comment has been minimized.

Copy link

commented May 18, 2017

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.

buildEnvironment(build, env);
}

public void buildEnvironment(Run<?,?> build, Map<String, String> env) {

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev May 18, 2017

Member

Same as for SVN plugin. It is better to reference the incoming method in the Core API.

This comment has been minimized.

Copy link
@abayer

abayer May 18, 2017

Author Member

Isn't that literally what I just did here?

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev May 18, 2017

Member

Not sure WDYM. I was talking about Javadoc and TODO comment

This comment has been minimized.

Copy link
@abayer

abayer May 19, 2017

Author Member

Ah, yeah, doing so.

@@ -16,7 +16,7 @@
<properties>
<jenkins.version>1.642.3</jenkins.version>
<no-test-jar>false</no-test-jar>
<scm-api-plugin.version>2.0.8</scm-api-plugin.version>
<scm-api-plugin.version>2.1.0</scm-api-plugin.version>

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev May 18, 2017

Member

I doubt it is required, but it won't hurt

@jglick

jglick approved these changes May 18, 2017

Copy link
Member

left a comment

Just please leave behind a comment reminding the maintainer (me) to delete the old overload and mark the new overload an @Override when updating to 2.60+.

@jglick

jglick approved these changes May 19, 2017

@jglick jglick merged commit 5cc6e1c into jenkinsci:master May 19, 2017

1 check passed

Jenkins This pull request looks good
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.