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

[JENKINS-58578] Force the setting of the private ip also for public subnet #387

Merged
merged 2 commits into from
Sep 25, 2019

Conversation

thoulen
Copy link
Contributor

@thoulen thoulen commented Aug 10, 2019

JENKINS-58578 Force the setting of the private ip also for public subnet

@thoulen thoulen requested a review from jvz August 10, 2019 20:12
@andgoldin
Copy link

This should probably also be updated in the provisionSpot function as well: Lines 950-954

Copy link

@andgoldin andgoldin left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@jvz
Copy link
Member

jvz commented Aug 13, 2019

Is this unit testable?

Copy link
Member

@jvz jvz left a comment

Choose a reason for hiding this comment

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

Code looks fine to me. I haven't done any manual testing.

Copy link
Contributor

@res0nance res0nance left a comment

Choose a reason for hiding this comment

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

Minor suggestions but looks good overall

@@ -633,11 +633,12 @@ private void logProvisionInfo(String message) {
}
}

net.setAssociatePublicIpAddress(false);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
net.setAssociatePublicIpAddress(false);
net.setAssociatePublicIpAddress(getAssociatePublicIp());

Will allow you to remove the if

@@ -946,11 +947,12 @@ private void setupCustomDeviceMapping(List<BlockDeviceMapping> deviceMappings) {
launchSpecification.setKeyName(keyPair.getKeyName());
launchSpecification.setInstanceType(type.toString());

net.setAssociatePublicIpAddress(false);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
net.setAssociatePublicIpAddress(false);
net.setAssociatePublicIpAddress(getAssociatePublicIp());

Same here

@varyvol
Copy link

varyvol commented Sep 25, 2019

Do you think this could be merged?

@res0nance res0nance changed the title Update SlaveTemplate.java [JENKINS-58578] Force the setting of the private ip also for public subnet Sep 25, 2019
@res0nance res0nance merged commit 816203b into jenkinsci:master Sep 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
5 participants