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-29817] Inject environment variables for a Job #56

Merged
merged 1 commit into from Aug 14, 2015

Conversation

Projects
None yet
7 participants
@recena
Copy link
Contributor

commented Aug 5, 2015

https://issues.jenkins-ci.org/browse/JENKINS-29817

  • Implement EnvInjectEnvVarsContributor
  • Add tests
  • Update the documentation (help information) for the Properties Content field

@reviewbybees

@recena recena changed the title [JENKINS-29817] Inject environment variables for a Job [WiP] [JENKINS-29817] Inject environment variables for a Job Aug 5, 2015

EnvVars environment = project.getEnvironment(jenkins.getInstance(), listener);

assertNotNull(environment.get("REPO"));
jenkins.assertBuildStatusSuccess(project.scheduleBuild2(0).get());

This comment has been minimized.

Copy link
@jglick

jglick Aug 5, 2015

Member

Running the build does not really accomplish anything. You could delete this line, or make the test demonstrate that the variable is also accessible to build steps. Which reminds me that it is likely that other code in this plugin is interpreting EnvInjectJobProperty on a Run (or AbstractBuild) that could probably now be deleted, since Run environments inherit from Job environments.

This comment has been minimized.

Copy link
@recena

recena Aug 5, 2015

Author Contributor

@jglick Indeed. I'm going to review the test.

This comment has been minimized.

Copy link
@recena

recena Aug 10, 2015

Author Contributor

@jglick Done


import static org.junit.Assert.assertNotNull;

public class EnvInjectEnvVarsJobTest {

This comment has been minimized.

Copy link
@jglick

jglick Aug 5, 2015

Member

Arguably should be called EnvInjectEnvVarsContributorTest.

This comment has been minimized.

Copy link
@recena

recena Aug 5, 2015

Author Contributor

@jglick I agree

This comment has been minimized.

Copy link
@recena

recena Aug 10, 2015

Author Contributor

@jglick Done

@jenkinsadmin

This comment has been minimized.

Copy link
Member

commented Aug 6, 2015

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


@Override
public void buildEnvironmentFor(Job job, EnvVars env, TaskListener listener) throws IOException, InterruptedException {
for (Object property : job.getProperties().values()) {

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Aug 6, 2015

Member

It's better to get properties by class without such cycles

This comment has been minimized.

Copy link
@recena

recena Aug 6, 2015

Author Contributor

@oleg-nenashev I'll take a look.

This comment has been minimized.

Copy link
@recena

recena Aug 11, 2015

Author Contributor

@oleg-nenashev I've done some changes related to your comment. Could you review it?

This comment has been minimized.

Copy link
@recena

recena Aug 11, 2015

Author Contributor

@oleg-nenashev I've reverted the changes because the tests were failing. I was trying this:

@Override
public void buildEnvironmentFor(Job job, EnvVars env, TaskListener listener) throws IOException, InterruptedException {
    EnvInjectJobProperty jobProperty = (EnvInjectJobProperty) job.getProperty(EnvInjectJobProperty.class)
    EnvInjectJobPropertyInfo jobPropertyInfo = jobProperty.getInfo();

    // Process "Properties Content"
    Map<String, String> result = jobPropertyInfo.getPropertiesContentMap(new HashMap<String, String>());
    if (result != null) {
        env.putAll(result);
    }
}

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Aug 13, 2015

Member

Null check is missing. If a job has no EnvInjectJobProperty, your test will fail with NPE

This comment has been minimized.

Copy link
@recena

recena Aug 13, 2015

Author Contributor

@oleg-nenashev AFAIK, if you are using the plugin (checkbox is clicked), there is always a EnvInjectJobProperty. And I've verified that job.getProperties() can never be null.

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Aug 13, 2015

Member

job.getProperty() may return null if you run the method agains the loaded job, which has been saved before the EnvInject installation.

job.getProperties() is never null, but IMO you're using a wrong pattern to lookup for properties. It may become performance-innefective in future Jenkins versions

This comment has been minimized.

Copy link
@recena

recena Aug 13, 2015

Author Contributor

@oleg-nenashev I don't know how job.getProperty() can be null if this method is invoked only if job.getProperties().values() returns values.

Do you prefer to use job.getProperty(EnvInjectJobProperty.class)?

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Aug 13, 2015

Member

Yes. And it can return the null value ;)

EnvInjectJobPropertyInfo jobPropertyInfo = jobProperty.getInfo();
Map<String, String> result = jobPropertyInfo.getPropertiesContentMap(new HashMap<String, String>());
if (result != null) {
env.putAll(result);

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Aug 6, 2015

Member

I doubt it will work as expected if a property depends on the value of another property. A test is required to demonstrate it

This comment has been minimized.

Copy link
@recena

recena Aug 6, 2015

Author Contributor

@oleg-nenashev There is a test.

@oleg-nenashev

This comment has been minimized.

Copy link
Member

commented Aug 6, 2015

I general I agree that such contributor mat be useful. On the other hand I cannot elaborate an impact of the change, because it may break the variable resolution order on existing installations. It may also cause a total breakage of existing jobs if they use Groovy scripts to inject variables and use build variables.

More tests and investigations are required before this PR becomes mergeable. I'm putting a 👎 on it now (as OSS contributor).

@recena

This comment has been minimized.

Copy link
Contributor Author

commented Aug 6, 2015

@oleg-nenashev What tests do you propose to verify that improvement (very needed) does not breaks anything?

This PR has been tested with Subversion Plugin (master) in order to verify this kind of use cases JENKINS-29340 and works perfectly.

Basically, we need the environment variables defined by EnvInject availables in Job.getEnvironment().

@oleg-nenashev

This comment has been minimized.

Copy link
Member

commented Aug 6, 2015

@recena
I'll try to respond on this evening.

@oleg-nenashev

This comment has been minimized.

Copy link
Member

commented Aug 8, 2015

@recena
After digging into the code...
EnvInjectInfo: getPropertiesContentMap() works with property contents only (text area field). See https://github.com/jenkinsci/envinject-plugin/blob/master/src/main/java/org/jenkinsci/plugins/envinject/EnvInjectInfo.java#L42-L65. The current implementation ignores environment variables from property files, Groovy scripts and from Master node.

Such implementation may address a narrow case from JENKINS-29817, but it is incomplete. I would not vote for merging the fix in the current state, because it may confuse the plugin users => serious 🐞 . You should implement a new method, which would generate variables without a build context.

I'm also aware about the variables resolution order. buildEnvironmentFor for Jobs is being called before the resolution for runs. According to the brief code analysis, seems that the use-case with build parameter overrides will be broken by the change. The behavior is configurable via the ''Override build parameters" option. According to the manual testing, AbstractBuild:getEnvironment() returns wrong values after the change. The test has been added in #58

@recena

This comment has been minimized.

Copy link
Contributor Author

commented Aug 10, 2015

@oleg-nenashev I'm going to implement the different way to provide variables (property files, groovy scripts, etc..). I don't understand why you say that this new extension breaks anything. The rest of tests are working fine. Maybe we should review these test, Isn't that so?

@recena

This comment has been minimized.

Copy link
Contributor Author

commented Aug 10, 2015

@oleg-nenashev There are a lot of users using Subversion Plugin and EnvInject Plugin. We need the environment variables (injected by this plugin) availables, p.e, in polling process. What would it your proposal?

@oleg-nenashev

This comment has been minimized.

Copy link
Member

commented Aug 11, 2015

I don't understand why you say that this new extension breaks anything

I just see a wrong output in the API method. This result needs more investigation. buildEnvironmentForJob() is being called before the Environment contributing actions, so probably the issue in overrides (or I'm doing something wrong in the debugger). I'll write a unit test and submit it.

I'm going to implement the different way to provide variables (property files, groovy scripts, etc..).

Please be aware about Groovy ones. I suspect that many Users rely on build runtime variables there. The plugin's code correctly handles null build references, but we should ensure that the newly introduced method is not failing horribly in the case of the misusage.

@oleg-nenashev

This comment has been minimized.

Copy link
Member

commented Aug 11, 2015

The rest of tests are working fine. Maybe we should review these test, Isn't that so?

Unfortunately the plugin does not contain tests for EnvInjectJobProperty at all :(

@recena recena changed the title [WiP] [JENKINS-29817] Inject environment variables for a Job [JENKINS-29817] Inject environment variables for a Job Aug 12, 2015

<p>All the properties name will be accessible as environment variables by their names. You can use or override the
properties specified in the above properties file.</p>

<p>These environment variables (<strong>only these</strong>) will be available in the context Job.</p>

This comment has been minimized.

Copy link
@recena

recena Aug 12, 2015

Author Contributor

@oleg-nenashev What do you think?

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Aug 12, 2015

Member

What is a context Job? I don't know such term. In any case it makes sense to add several sentences, which would allow users to understand what's going on here.

@reviewbybees

This comment has been minimized.

Copy link

commented Aug 12, 2015

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.

<ul>
<li>Validate Subversion Repository with a URL like this: <code>http://server/svn/$REPO</code></li>
<li>Subversion SCM Polling</li>
</ul>
</div>

This comment has been minimized.

Copy link
@recena

recena Aug 12, 2015

Author Contributor

@oleg-nenashev Better?

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Aug 12, 2015

Member

My proposals:

... will be available in the Job environment independently from the build. There are following use-cases: SCM polling, validation of fields depending on environment variables (e.g. http://server/svn/$REPO), custom workspace management, etc. 

Limitations: The field may contain variables, which are not resolvable outside the Build context (e.g. BUILD_NUMBER, WORKSPACE, etc.), the plugin [injects empty value/ignores the field/or something else]). This behavior should be taken into account by job editors.

Please also add a proper test-case for the limitation

This comment has been minimized.

Copy link
@recena

recena Aug 12, 2015

Author Contributor

@oleg-nenashev Thanks so much for your help. I'm going to investigate the limitations.

This comment has been minimized.

Copy link
@recena

recena Aug 12, 2015

Author Contributor

@oleg-nenashev I don't understand what you mean with limitations. Could you show an example the variable defined in this field not resolvable? Do you mean something like this VAR1=$WORKSPACE?

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Aug 12, 2015

Member

@recena
Yes, WORKSPACE var is a valid example for this limitation

@recena

This comment has been minimized.

Copy link
Contributor Author

commented Aug 12, 2015

@oleg-nenashev What do you thing about last commit?

@@ -35,6 +21,20 @@
value="${instance.info.groovyScriptContent}"/>
</f:entry>

<f:entry field="propertiesFilePath"

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Aug 13, 2015

Member

Why did you move this section? How is it related to this PR?

This comment has been minimized.

Copy link
@recena

recena Aug 13, 2015

Author Contributor

@oleg-nenashev Indeed this change is not directly related to PR but I considered it would be helpful to show the form fields in the same order in both sections. Don't make sense to use a different order.

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Aug 13, 2015

Member

NIT: "Properties content" is the most widely used section, so I would use another direction. BTW, the interface should be reworked to show only the required section (extension point?). Such rework is not a part of the PR, so I'd prefer to submit it as another PR

This comment has been minimized.

Copy link
@recena

recena Aug 13, 2015

Author Contributor

@oleg-nenashev We can discuss about it out of the PR.

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Aug 13, 2015

Member

As a summary of the discussion, I still think it should be moved to another PR:

  1. It is completely unrelated to the PR
  2. The change impacts the usability and UX IMO, because it changes the original layout
@@ -0,0 +1,6 @@
<div>

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Aug 13, 2015

Member

Seems you've forgotten to remove the original help files

This comment has been minimized.

Copy link
@recena

recena Aug 13, 2015

Author Contributor

@oleg-nenashev We need both help files because we can use this plugin in different ways:

  • Prepare an environment for the run: ...set an environment for the build job (before)
  • Inject environment variables to the build process: ... to the whole build process. (during)

Please, find attached this screenshot:

jenkins-29817

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Aug 13, 2015

Member

Now I get the reason. In such case you should reference a text with extra details by URL. It will prevent confusions of users

This comment has been minimized.

Copy link
@recena

recena Aug 13, 2015

Author Contributor

@oleg-nenashev I don't know what you mean with "...reference a text with extra details by URL."

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Aug 13, 2015

Member

I just mean it makes sense to put the extended help message to resources and reference it explicitly

This comment has been minimized.

Copy link
@recena

recena Aug 13, 2015

Author Contributor

@olamy In this case, I prefer to leave at that.

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Aug 13, 2015

Member

OK, agreed

<p>
KEY=VALUE形式で、キーと値を設定します。<br />
環境変数にプロパティの名前でアクセスできます。<br />
上記のプロパティファイルで設定したプロパティを使用したり、上書きすることも可能です。

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Aug 13, 2015

Member

I suspect it's the original message. It makes sense to remove it (users will get the English text) or ask somebiody like @kohsuke to translate it. Otherwise users with Japanese locale will get a message without sensitive details

This comment has been minimized.

Copy link
@lanwen

lanwen Aug 13, 2015

Member

maybe just use some translate service?

@@ -5,7 +5,7 @@
<parent>
<groupId>org.jenkins-ci.plugins</groupId>
<artifactId>plugin</artifactId>
<version>1.532</version>
<version>1.532.3</version>

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Aug 13, 2015

Member

Yes, the original version makes me sad as well (unit tests do not work reliably)

@oleg-nenashev

This comment has been minimized.

Copy link
Member

commented Aug 13, 2015

🐛 due to the unrelated change in src/main/resources/org/jenkinsci/plugins/envinject/EnvInjectBuildWrapper/config.jelly, which is not related to this PR. It should be decoupled to a separate pull-request

@amuniz

This comment has been minimized.

Copy link
Member

commented Aug 13, 2015

@oleg-nenashev Is that really a bug? Is preventing to use the plugin somehow? Since it's not really a functional change and does not affect in any way to the behavior being modified in this PR, it's ok for me. Moreover it's a minor change that you can quick see that is only a code block moved some lines below.

@oleg-nenashev

This comment has been minimized.

Copy link
Member

commented Aug 13, 2015

Please also address a comment regarding the Japanese help

@oleg-nenashev

This comment has been minimized.

Copy link
Member

commented Aug 13, 2015

🐝
Could you please squash commits? After that we will be able to merge it according to the policy. I'm planning to release 0.92-beta1 on the weekend (or today if everything goes right)

@recena

This comment has been minimized.

Copy link
Contributor Author

commented Aug 13, 2015

@recena

This comment has been minimized.

Copy link
Contributor Author

commented Aug 13, 2015

@oleg-nenashev By the way, Is there an official maintainer? /cc @gboissinot

recena added a commit that referenced this pull request Aug 14, 2015

Merge pull request #56 from recena/JENKINS-29817
[JENKINS-29817] Inject environment variables for a Job

@recena recena merged commit 95c1f0e into jenkinsci:master Aug 14, 2015

1 check passed

Jenkins This pull request looks good
Details

TaskListener listener = jenkins.createTaskListener();
EnvVars environment = project.getEnvironment(jenkins.getInstance(), listener);
assertEquals("${WORKSPACE}", environment.get("VAR1"));

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Aug 16, 2015

Member

This check is failing on my local laptop. The result is null

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Aug 17, 2015

Member

fixed by clean build, please ignore

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.