[util] Fix get_hostname for windows ec2 instances #2451
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| # try and find a EC2 instance ID | ||
| if (Platform.is_ecs_instance()) or \ | ||
| (hostname is not None and True in [hostname.lower().startswith(p) for p in [u'ip-', u'domu']]) or \ | ||
| (hostname is None and os_name == 'windows'): | ||
| instanceid = EC2.get_instance_id(config) |
There was a problem hiding this comment.
Up to this bloc, the code never attempts to resolve the Windows hostname as _get_hostname_unix is called on UNIX system only. The hostname == None, and we have no clue whether the host belongs or not to EC2.
It'd be better if we can get a sense of it and spare undesired call to the EC2.get_instance_id method. We could for instance eval the socket.gethostname value sooner on Windows systems.
hkaj
force-pushed
the
haissam/get_hostname_fix
branch
from
d9663d4 to
dab517f
Compare
|
@yannmh Thanks for the feedback, |
|
Looks good. Thanks @hkaj. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Right now
get_hostnamenever resolves to an EC2 instance id for windows host because the condition to look for it didn't include empty hostnames. This PR fixes that.