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

Do not tag instances if no serverUrl #319

Merged
merged 1 commit into from
Nov 13, 2018

Conversation

batmat
Copy link
Member

@batmat batmat commented Nov 7, 2018

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

This is probably more sensible than my previous attempt on #314, and in the end matches what @thoulen was saying would probably have been preferrable: we just do not tag in case there's no serverUrl.

It's also a followup of #310, for inter-tracking purpose.

Anyway the number of production instances out there without a serverUrl is probably close to 0, but well.

This is probably more sensible than my previous attempt.
*Anyway* the number of production instances out there without a serverUrl
is probably close to 0, but well.
@batmat
Copy link
Member Author

batmat commented Nov 7, 2018

Tested successfully by building a local package and manual upload. Going to do a second check on a freshly provisioned setup as soon as ci.jenkins.io builds the incremental release for this PR.

@batmat batmat changed the title [WIP] Do not tag instances if no serverUrl Do not tag instances if no serverUrl Nov 7, 2018
@batmat
Copy link
Member Author

batmat commented Nov 7, 2018

Just confirmed this works on a fresh instance. Sorry for the mishaps on #314.

@@ -377,8 +367,8 @@ private int countCurrentEC2Slaves(SlaveTemplate template) throws AmazonClientExc
jenkinsServerUrl = jenkinsLocation.getUrl();

if (jenkinsServerUrl == null) {
LOGGER.log(Level.WARNING, "No Jenkins server URL specified, using instance-identity instead");
jenkinsServerUrl = getInstanceIdentityEncodedPublicKey();
Copy link
Member Author

Choose a reason for hiding this comment

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

not trying anymore to be smart for no good reason, just warn and not crash if serverUrl is unspecified.

@imod
Copy link
Member

imod commented Nov 9, 2018

any chance we can get a release with this soon?

@thoulen
Copy link
Contributor

thoulen commented Nov 9, 2018

We should release in the beginning of next week, i have two pending task the pr 321 and fix the CAP

@thoulen thoulen merged commit 9d8bf41 into jenkinsci:master Nov 13, 2018
@batmat batmat deleted the JENKINS-54266-second-attempt branch November 13, 2018 11:56
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.

4 participants