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-42381] Add PATH+GRADLE and buildEnvVars(...) #42

Closed
wants to merge 2 commits into from

Conversation

abayer
Copy link
Member

@abayer abayer commented Feb 28, 2017

JENKINS-42381

This ensures that Gradle'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 and @wolfs

This ensures that Gradle'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.
@ghost
Copy link

ghost 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.

Copy link
Member

@jglick jglick left a comment

Choose a reason for hiding this comment

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

Would best have test coverage.

@wolfs
Copy link
Member

wolfs commented Feb 28, 2017

Thanks for your pull request.

Could you add an integration test exposing the problem you fix here?

def envVars = new EnvVars()
installation.buildEnvVars(envVars)
assert envVars.containsKey("PATH+GRADLE")
assert envVars.get("PATH+GRADLE") == installation.home + "/bin"
Copy link
Member

Choose a reason for hiding this comment

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

This is a unit test. An integration test would actually demonstrate that $PATH is right.

Copy link
Member Author

Choose a reason for hiding this comment

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

Touché. Lemme figure out how I can test the PATH here.

Copy link
Member

Choose a reason for hiding this comment

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

Since the stated use case is for Declarative, perhaps you should have that as a test dep and demonstrate that

tools {…}
steps {
  echo "PATH=$PATH"
}

works.

Copy link
Member Author

Choose a reason for hiding this comment

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

Downside to that is that we'd have to bump the Gradle plugin's core dependency to 2.7.1 from its current 1.642.1.

Copy link
Member

@wolfs wolfs left a comment

Choose a reason for hiding this comment

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

I really would like to have a better integration test showing what the effect of "PATH+GRADLE" as environment variable would be and what the use case is for adding this environment variable.

def envVars = new EnvVars()
installation.buildEnvVars(envVars)
assert envVars.containsKey("PATH+GRADLE")
assert envVars.get("PATH+GRADLE") == installation.home + "/bin"
Copy link
Member

Choose a reason for hiding this comment

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

By an integration test I rather meant that the path to GRADLE is really added to the path and then we can execute gradle. Preferable in a pipeline job. Is that possible?

Copy link
Member Author

Choose a reason for hiding this comment

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

Lemme see what I can do.

@abayer
Copy link
Member Author

abayer commented Feb 28, 2017

@wolfs @jglick The only way I can see to do an integration test is by bumping the core dependency to 2.7.1 so we can use Declarative as a test dependency. As I said over on the related Ant PR (jenkinsci/ant-plugin#26 (comment)), I think that verifying that PATH+GRADLE gets added to the environment by buildEnvVars is sufficient - that's exactly the path that Declarative exercises (https://github.com/jenkinsci/pipeline-model-definition-plugin/blob/master/pipeline-model-definition/src/main/java/org/jenkinsci/plugins/pipeline/modeldefinition/steps/EnvVarsForToolStep.java#L130-L136) and passes onward to withEnv. But, your call(s).

@wolfs
Copy link
Member

wolfs commented Feb 28, 2017

@abayer Is it possible to build a plugin against one version of Jenkins and test against a few other versions? Could I just create a few test configurations containing different Jenkins versions and it would work?

@abayer
Copy link
Member Author

abayer commented Feb 28, 2017

@wolfs I honestly don't know. =)

@jglick
Copy link
Member

jglick commented Feb 28, 2017

Is it possible to build a plugin against one version of Jenkins and test against a few other versions?

No. You can run tests against whatever versions you like, by passing -Djenkins.version=… on the command line, but you cannot write tests against any version other than your actual baseline. So I guess this is a dead end.

@wolfs
Copy link
Member

wolfs commented Mar 9, 2017

@abayer Well, looks like this is all the test coverage I get for now. Could you please add something to the release notes in the README?

wolfs added a commit that referenced this pull request Apr 2, 2017
[FIXED JENKINS-42381] Add PATH+GRADLE and buildEnvVars(...)
@wolfs
Copy link
Member

wolfs commented Apr 2, 2017

This has been merged manually by e195cf3.

@wolfs wolfs closed this Apr 2, 2017
@wolfs
Copy link
Member

wolfs commented Apr 2, 2017

Thank you for your contribution @abayer!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants