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

[FIX JENKINS-23007] Consider environment when polling. #83

Closed
wants to merge 3 commits into from

Conversation

@daniel-beck
Copy link
Member

commented May 13, 2014

Otherwise, the credentials for locations with variables in their URL
aren't considered, requiring a fallback to Additional Credentials.

[FIX JENKINS-23007] Consider environment when polling.
Otherwise, the credentials for locations with variables in their URL
aren't considered, requiring a fallback to Additional Credentials.
@cloudbees-pull-request-builder

This comment has been minimized.

Copy link

commented May 13, 2014

plugins » subversion-plugin #357 SUCCESS
This pull request looks good

@jglick

This comment has been minimized.

Copy link
Member

commented May 13, 2014

Might be better to use the new API that lets you get an environment from a Job rather than only a Run. (Needs a newer core dep.)

@jenkinsadmin

This comment has been minimized.

Copy link
Member

commented May 14, 2014

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

@daniel-beck

This comment has been minimized.

Copy link
Member Author

commented May 14, 2014

@jglick Done. (Unfortunately no @since so it took me a bit to find what you mean…)

I'd have preferred to not specify a node, but Job.getEnvironment(Node,TaskListener) gets Jenkins.getGlobalNodeProperties() only when it's non-null.

@cloudbees-pull-request-builder

This comment has been minimized.

Copy link

commented May 14, 2014

plugins » subversion-plugin #358 SUCCESS
This pull request looks good

jglick added a commit to jenkinsci/jenkins that referenced this pull request May 14, 2014
@daniel-beck

This comment has been minimized.

Copy link
Member Author

commented May 23, 2014

@jglick: Any way I can get the current environment including global env vars without this kind of crap? Or can this be considered a bug in Jenkins that it requires a node to be specified to get node-independent env vars?

May 23, 2014 5:31:52 PM SEVERE hudson.triggers.SCMTrigger$Runner runPolling
Failed to record SCM polling for hudson.model.FreeStyleProject@218d9941[JobName]
java.lang.IllegalArgumentException: Node ABC seems to be offline
            at hudson.tools.ToolInstaller.preferredLocation(ToolInstaller.java:120)
            at hudson.tools.JDKInstaller.performInstallation(JDKInstaller.java:109)
            at hudson.tools.InstallerTranslator.getToolHome(InstallerTranslator.java:61)
            at hudson.tools.ToolLocationNodeProperty.getToolHome(ToolLocationNodeProperty.java:107)
            at hudson.tools.ToolInstallation.translateFor(ToolInstallation.java:204)
            at hudson.model.JDK.forNode(JDK.java:126)
            at hudson.model.AbstractProject.getEnvironment(AbstractProject.java:359)
            at hudson.scm.SubversionSCM.compareRemoteRevisionWith(SubversionSCM.java:1402)
            at hudson.scm.SCM._compareRemoteRevisionWith(SCM.java:356)
            at hudson.scm.SCM.poll(SCM.java:373)
            at hudson.model.AbstractProject._poll(AbstractProject.java:1567)
            at hudson.model.AbstractProject.poll(AbstractProject.java:1490)
            at hudson.triggers.SCMTrigger$Runner.runPolling(SCMTrigger.java:450)
            at hudson.triggers.SCMTrigger$Runner.run(SCMTrigger.java:479)
            at hudson.util.SequentialExecutionQueue$QueueEntry.run(SequentialExecutionQueue.java:118)
            at java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:471)
            at java.util.concurrent.FutureTask.run(FutureTask.java:262)
            at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1145)
            at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:615)
            at java.lang.Thread.run(Thread.java:745)
@jglick

This comment has been minimized.

Copy link
Member

commented May 23, 2014

I think AbstractProject.getEnvironment is to blame here:

        if (node != null) { // just in case were not in a build
            jdk = jdk.forNode(node, listener);
        }

should also check toComputer to make sure it is non-null, and check isOnline as well.

@cloudbees-pull-request-builder

This comment has been minimized.

Copy link

commented May 24, 2014

plugins » subversion-plugin #363 FAILURE
Looks like there's a problem with this pull request

[JENKINS-23007] Use environment of the node that polls
Jenkins cannot provide the environment of an offline node because
it throws while building the JDK path. So the last built node can't
be used for this. Instead, use the polling node's environment.

While this might still have unexpected results, it seems to be a
saner approach compared to building up the environment from
Jenkins.getGlobalNodeProperties()/AbstractProject.getEnvironment(...).
@cloudbees-pull-request-builder

This comment has been minimized.

Copy link

commented May 24, 2014

plugins » subversion-plugin #364 SUCCESS
This pull request looks good

@jglick

This comment has been minimized.

Copy link
Member

commented Jun 24, 2014

Filed as JENKINS-23517.

@oleg-nenashev

This comment has been minimized.

Copy link
Member

commented Oct 18, 2014

@jglick , @daniel-beck
JENKINS-23517 has been fixed and released. Are you going to merge PR and bump the plugin's core dependency to 1.580.x?

@jglick

This comment has been minimized.

Copy link
Member

commented Oct 18, 2014

Was backported to 1.565.1, it seems.

@daniel-beck

This comment has been minimized.

Copy link
Member Author

commented Oct 18, 2014

Doesn't really make a difference. 2.4.x is still on 1.509, while master is on 1.568.

@recena

This comment has been minimized.

Copy link
Contributor

commented Jul 19, 2015

@daniel-beck I've thought it would be interesting to close PR. I've filled a Jira ticket and I've sent a new PR in order to resolve the same problem. Do you agree?

@recena

This comment has been minimized.

Copy link
Contributor

commented Jul 21, 2015

@daniel-beck This PR isn't valid because this method was refactoring recently.

@recena recena closed this Jul 21, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.