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

[FIXED JENKINS-42382] Add PATH+ANT to buildEnvVars(...) #26

Merged
merged 2 commits into from May 9, 2017

Conversation

Projects
None yet
4 participants
@abayer
Copy link
Member

commented Feb 28, 2017

JENKINS-42382

This ensures that Ant's bin directory ends up on the PATH when using
the Pipeline tool step. Note that Jenkins' environment variable
management will handle converting "/bin" on Windows if needed, we do
not need to do that explicitly.

cc @reviewbybees esp @armfergom

[FIXED JENKINS-42382] Add PATH+ANT to buildEnvVars(...)
This ensures that Ant's bin directory ends up on the PATH when using
the Pipeline tool step. Note that Jenkins' environment variable
management will handle converting "/bin" on Windows if needed, we do
not need to do that explicitly.
@reviewbybees

This comment has been minimized.

Copy link

commented Feb 28, 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.

@jglick

jglick approved these changes Feb 28, 2017

Copy link
Member

left a comment

This really ought to get test coverage, especially if you claim this is a regression.

@abayer

This comment has been minimized.

Copy link
Member Author

commented Feb 28, 2017

@jglick It's not a regression - fixed the JIRA to be an improvement.

@@ -123,6 +124,12 @@ private void verify() throws Exception {
assertEquals(1,l.length);
r.assertEqualBeans(l[0],new AntInstallation("myAnt","/tmp/foo", JenkinsRule.NO_PROPERTIES),"name,home");

// Verify that PATH+ANT is set.

This comment has been minimized.

Copy link
@jglick

jglick Feb 28, 2017

Member

That is not what we actually care about. We want to know if a full environment, once expanded, contains a PATH which starts with the new dir.

This comment has been minimized.

Copy link
@abayer

abayer Feb 28, 2017

Author Member

Isn't that already established enough in core? Legitimately wondering here - such a test would not be testing anything specific to Ant, it'd be testing core's PATH+FOO logic, which is already covered in https://github.com/jenkinsci/jenkins/blob/master/core/src/test/java/hudson/EnvVarsTest.java#L51-L69

This comment has been minimized.

Copy link
@jglick

jglick Feb 28, 2017

Member

Of course core unit-tests that behavior, but this would be proving that the purported fix actually has some effect: that the method you are overriding is called when you think it is, etc.

This comment has been minimized.

Copy link
@abayer

abayer Feb 28, 2017

Author Member

I'll be blunt - I don't think that's necessary. If buildEnvVars results in PATH+ANT being included, that seems sufficient to me, since we know that buildEnvVars gets called and that PATH+FOO in EnvVars gets added to the PATH.

This comment has been minimized.

Copy link
@jglick

jglick Feb 28, 2017

Member

OK. Can I assume you are doing at least manual testing?

pipeline-model-definition could add a test-scoped dep on this and do a full integration test there, if you also add a test dep on jenkins-test-harness-tools.

This comment has been minimized.

Copy link
@abayer

abayer Feb 28, 2017

Author Member

Yup, did manual testing, and I'd happily add a test in pipeline-model-definition.

@armfergom armfergom merged commit d1c5e13 into jenkinsci:master May 9, 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.