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

Slave.JnlpJar.getURL did not work in some modes when core had a snapshot dependency on Remoting #3069

Merged
merged 3 commits into from Oct 20, 2017

Conversation

3 participants
@jglick
Member

jglick commented Oct 9, 2017

When experimenting with remoting changes using a snapshot dependency, I found that two kinds of things did not work:

  • JenkinsRule.createSlave and similar overloads would fail to find remoting.jar in the webapp context and fail.
  • mvn -f war hudson-dev:run would also fail to find remoting.jar when creating agents interactively.

Also, lookup of jenkins-cli.jar seemed to be fragile at best (sensitive to Maven process CWD).

Changing both to verify what they are returning more carefully. If a true JAR can be found in the webapp context, fine—use it.

Otherwise replace target/classes/ with the matching target/whatever.jar assembly. (This is also more convenient for iteractively testing changes to that component: you can simply mvn -f ../remoting -DskipTests package or mvn -f cli -DskipTests package to rebuild that JAR, and have it take effect in a functional test or interactive debugging session, without touching core or war modules.)

And if neither works, fail with a meaningful error, rather than guessing at the process CWD and returning a potentially nonexistent URL.

Proposed changelog entries

None needed, should not affect production usage.

Desired reviewers

@reviewbybees

@jglick jglick requested review from stephenc and oleg-nenashev Oct 9, 2017

@reviewbybees

This comment has been minimized.

reviewbybees commented Oct 9, 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.

@jglick

This comment has been minimized.

Member

jglick commented Oct 10, 2017

Apparent flake:

org.junit.runners.model.TestTimedOutException: test timed out after 180 seconds
	at java.lang.Thread.sleep(Native Method)
	at hudson.model.QueueTest.shouldBeAbleToBlockFlyweightTaskAtTheLastMinute(QueueTest.java:865)

Prior to #3055 this would have hung the whole build, I think, so it seems that patch worked.

@oleg-nenashev

I see nothing wrong with that 🐝

@jglick

This comment has been minimized.

Member

jglick commented Oct 12, 2017

@oleg-nenashev

This comment has been minimized.

Member

oleg-nenashev commented Oct 13, 2017

I doubt the test failure is somehow related, retriggering just in case

@oleg-nenashev

This comment has been minimized.

Member

oleg-nenashev commented Oct 14, 2017

retriggering CI

@oleg-nenashev

This comment has been minimized.

Member

oleg-nenashev commented Oct 15, 2017

Issues with Update center appear consistently, but I doubt this PR is a culprit. Will put on-hold for investigation

@jglick jglick removed the on-hold label Oct 17, 2017

@oleg-nenashev oleg-nenashev merged commit 50eb7f1 into jenkinsci:master Oct 20, 2017

1 check passed

continuous-integration/jenkins/pr-head This commit looks good
Details

bkmeneguello pushed a commit to bkmeneguello/jenkins that referenced this pull request Oct 23, 2017

@jglick jglick deleted the jglick:JnlpJar branch Oct 23, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment