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

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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

kazesberger
Copy link

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.

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.
@buildhive
Copy link

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

@kazesberger
Copy link
Author

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.

Property And Run On Slaves without overwriting "real" environment
variables like PATH etc.

wrote integration and unit test for this feature.

added test-scoped dependency to powermock framework to be able to mock
dependencies to static calls.
@kazesberger
Copy link
Author

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
Copy link

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

@jenkinsadmin
Copy link
Member

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

@jglick
Copy link
Member

jglick commented Jul 29, 2013

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.

@oleg-nenashev oleg-nenashev changed the title https://issues.jenkins-ci.org/browse/JENKINS-16316 [nO_MERGE] [JENKINS-16316] - Bug fix and additional tests Mar 8, 2015
@oleg-nenashev oleg-nenashev changed the title [nO_MERGE] [JENKINS-16316] - Bug fix and additional tests [NO_MERGE] [JENKINS-16316] - Bug fix and additional tests Mar 8, 2015
@recena
Copy link
Contributor

recena commented Aug 17, 2015

@kazesberger Do you agree if I close this PR?

@oleg-nenashev
Copy link
Member

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

@recena
Copy link
Contributor

recena commented Aug 18, 2015

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

@oleg-nenashev
Copy link
Member

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

@recena
Copy link
Contributor

recena commented Aug 18, 2015

@oleg-nenashev I'll do it.

Set<Entry<String, String>> afterChange = hudson.getComputer(slaveNode.getNodeName()).getEnvironment().entrySet();
// environment of the slave does not change without restarting it. assert it to make test fail if there will be
// some kind of auto-restart to reload config.
Assert.assertEquals(beforeChange, afterChange);

Choose a reason for hiding this comment

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

MAJOR Add a message to this assertion. rule

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

Successfully merging this pull request may close these issues.

8 participants