[NO_MERGE] [JENKINS-16316] - Bug fix and additional tests

[kazesberger]

slave (which are outdated if you dont restart slave nodes after changing global properties)

CAUTION - this might break some features: EnvironmentContributors don't get recognized any more (because CoreEnvironmentContributor did a Computer.current.getEnvironment() which also leads to overwriting with outdated values.

how to reproduce the problem: hpi:run on current head with jenkins-version set to "1.480.3" (current LTS verison) configure a global property on master K/V: TESTVAR/value1 configure a slave and start it configure a job restricted to run on that slave and to echo $TESTVAR. run the job (it will echo "value1") configure the global property on master to be: TESTVAR/value2 dont restart anything rerun the job (it will still echo "value1") however a job on master will recognize the changed value. there are 2 pieces of code that lead to this behaviour. I'm pretty uncertain about the fix i made, but it resolves at least our problem with the plugin as i described it above, although i'm pretty sure it breaks some other intended feature. https://github.com/kazesberger/envinject-plugin.git https://github.com/kazesberger/envinject-plugin/commits/master maybe someone can have a look at it and/or tell me what's the story behind this (Core)EnvironmentContributor thing.

[buildhive]

Jenkins » envinject-plugin #91 FAILURE Looks like there's a problem with this pull request (what's this?)

[kazesberger]

oh i didnt realize that this should be/stay the minimum jenkins version. i changed the version because i wanted to test/code for our Jenkins which is at current LTS and thought it would be good to be at least compatible with LTS in general.

[kazesberger]

so finally i had enough time to make a real fix to this and cover my changes with unit and integration test. it should be very clear what these changes do if you take a look at the tests.

[buildhive]

Jenkins » envinject-plugin #92 UNSTABLE Looks like there's a problem with this pull request (what's this?)

[jenkinsadmin]

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

[jglick]

Since #20 has been merged, perhaps you should close this and refile just the test parts, to confirm that the fix works according to your tests? Or perhaps a new functional (not PowerMock-based) test that creates a real job printing an environment variable and verifies that global changes to it are honored? (I would recommend committing the test against envinject-1.87 as a parent, verifying that it fails in the expected way, then merge into master, verifying that it now passes.) I suppose I could have done this myself, I was just too lazy.

[recena]

@kazesberger Do you agree if I close this PR?

[oleg-nenashev]

@recena Please don't rush. The PR contains a useful test, which may be decoupled. That's why we keep this PR open

[recena]

@oleg-nenashev Ok, Do you agree if we move the interested tests to a new PR and close this?

[oleg-nenashev]

@recena I definitely agree. I was just too lazy to do it, so I kept the PR :)

[recena]

@oleg-nenashev I'll do it.

[dummy-lanwen-bot]

SonarQube analysis reported 22 issues:

• 1 blocker
• 2 critical
• 15 major
• 4 minor

Watch the comments in this conversation to review them. Note: the following issues could not be reported as comments because they are located on lines that are not displayed in this pull request:

• Split this 139 characters long line (which is greater than 120 authorized).
• Split this 138 characters long line (which is greater than 120 authorized).
• Either log or rethrow this exception.
• Split this 136 characters long line (which is greater than 120 authorized).
• Catch Exception instead of Throwable.
• Either log or rethrow this exception.
• Make this a named "static" inner class.
• Missing curly brace.
• Remove this unused "LOG" private field.
• Add a message to this assertion.
• Add a message to this assertion.