Implement ServerWithNatRuleToNodeMetadata Function #428
Conversation
| Server server = serverWithExternalIp.server(); | ||
| builder.ids(server.id()); | ||
| builder.name(server.name()); | ||
| builder.hostname(serverWithExternalIp.server().description()); |
There was a problem hiding this comment.
Is this really the hostname? Don't put that here if not.
| builder.ids(server.id()); | ||
| builder.name(server.name()); | ||
| builder.hostname(serverWithExternalIp.server().description()); | ||
| if (server.datacenterId() != null) { |
There was a problem hiding this comment.
Under which circumstances can this be null?
There was a problem hiding this comment.
Should not be null. Our xsd has it as a required attribute:
<xs:attribute name="datacenterId" type="xs:string" use="required"/>
| } | ||
| if (privateAddress != null && serverWithExternalIp.externalIp() != null) { | ||
| builder.publicAddresses(ImmutableSet.of(serverWithExternalIp.externalIp())); | ||
| } |
There was a problem hiding this comment.
We should consider not only the primary NIC here. Let's return all IP addresses of the server.
There was a problem hiding this comment.
BTW, why read the external IP only if there is a private IP?
| Credentials credentials = credentialStore.get("node#" + server.id()); | ||
| if (credentials instanceof LoginCredentials) { | ||
| builder.credentials(LoginCredentials.class.cast(credentials)); | ||
| } |
There was a problem hiding this comment.
Remove this. The credentials are already set by jclouds.
| BaseImage getBaseImage(final String sourceImageId) { | ||
| BaseImage baseImage = api.getServerImageApi().getOsImage(sourceImageId); | ||
| return baseImage == null ? api.getServerImageApi().getCustomerImage(sourceImageId) : baseImage; | ||
| } |
There was a problem hiding this comment.
This means two additional calls for each existing node, to get the image info. Instead of doing this, could you inject the image supplier to this class?
jclouds caches the results of the ComptueService.listImages call (as it is usually an expensive one). You could look for the image in the list of cached images and save these two calls for each node (that would be a significant amount of extra calls when calling listNodes in large environments).
|
ServerWithNatRuleToNodeMetadata Function reimplemented without expensive image list API calls. |
There was a problem hiding this comment.
Thanks @btrishkin! Just one minor comment left. Apart from that, LGTM!
|
|
||
| private final OperatingSystemToOsFamily operatingSystemToOsFamily; | ||
|
|
||
| public OperatingSystemToOperatingSystem(final OperatingSystemToOsFamily operatingSystemToOsFamily) { |
There was a problem hiding this comment.
Add the @Inject annotation and remove the public modifier from the constructor to make it package private (background).
btrishkin
force-pushed
the
server-with-nat-rule-to-node-metadata
branch
from
69cdeb3
to
39a47e7
Compare
|
@nacx small issue fixed and commits squashed. |
|
Pushed to master and 2.1.x. Thanks @btrishkin! |
No description provided.