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

More portable extraction of short hostname #13109

Merged
merged 1 commit into from Aug 25, 2015

Conversation

Projects
None yet
3 participants
@jasontedor
Member

jasontedor commented Aug 25, 2015

This commit increases the portability of extracting the short hostname
on a Unix-like system.

Closes #13107

@nik9000

This comment has been minimized.

Show comment
Hide comment
@nik9000

nik9000 Aug 25, 2015

Contributor

This probably deserves a comment or some helpful person will revert it back to hostname -s. Which unices don't have -s, btw?

Contributor

nik9000 commented Aug 25, 2015

This probably deserves a comment or some helpful person will revert it back to hostname -s. Which unices don't have -s, btw?

@nik9000

This comment has been minimized.

Show comment
Hide comment
@nik9000

nik9000 Aug 25, 2015

Contributor

This probably deserves a comment or some helpful person will revert it back to hostname -s. Which unices don't have -s, btw?

This probably deserves a comment or some helpful person will revert it back to hostname -s. Which unices don't have -s, btw?

Ah. Solaris 10.

Contributor

nik9000 commented Aug 25, 2015

This probably deserves a comment or some helpful person will revert it back to hostname -s. Which unices don't have -s, btw?

This probably deserves a comment or some helpful person will revert it back to hostname -s. Which unices don't have -s, btw?

Ah. Solaris 10.

@nik9000

This comment has been minimized.

Show comment
Hide comment
@nik9000

nik9000 Aug 25, 2015

Contributor

Otherwise LGTM.

Contributor

nik9000 commented Aug 25, 2015

Otherwise LGTM.

More portable extraction of short hostname
This commit increases the portability of extracting the short hostname
on a Unix-like system.

Closes #13107
@jasontedor

This comment has been minimized.

Show comment
Hide comment
@jasontedor

jasontedor Aug 25, 2015

Member

@nik9000 Also HP-UX.

I've added a comment as you requested. I also discovered in the course of testing this is on Solaris VM that combining the variable definition and export into a single line isn't supported in some shells (namely the Bourne shell which is the default shell on Solaris). I've found a more portable way to do the export as well. I've updated the pull request accordingly.

Member

jasontedor commented Aug 25, 2015

@nik9000 Also HP-UX.

I've added a comment as you requested. I also discovered in the course of testing this is on Solaris VM that combining the variable definition and export into a single line isn't supported in some shells (namely the Bourne shell which is the default shell on Solaris). I've found a more portable way to do the export as well. I've updated the pull request accordingly.

@nik9000

This comment has been minimized.

Show comment
Hide comment
@nik9000

nik9000 Aug 25, 2015

Contributor

I've found a more portable way to do the export as well. I've updated the pull request accordingly.

Bleh. I'm not against just using #!/bin/bash instead of #!/bin/sh.

Thanks for adding the comment - that is perfect.

Contributor

nik9000 commented Aug 25, 2015

I've found a more portable way to do the export as well. I've updated the pull request accordingly.

Bleh. I'm not against just using #!/bin/bash instead of #!/bin/sh.

Thanks for adding the comment - that is perfect.

@jasontedor

This comment has been minimized.

Show comment
Hide comment
@jasontedor

jasontedor Aug 25, 2015

Member

I'm not against just using #!/bin/bash instead of #!/bin/sh.

@nik9000 I've been contemplating that as well but at a minimum it is a separate issue from this one.

Member

jasontedor commented Aug 25, 2015

I'm not against just using #!/bin/bash instead of #!/bin/sh.

@nik9000 I've been contemplating that as well but at a minimum it is a separate issue from this one.

jasontedor added a commit that referenced this pull request Aug 25, 2015

Merge pull request #13109 from jasontedor/fix/13107
More portable extraction of short hostname

@jasontedor jasontedor merged commit 9ceca39 into elastic:master Aug 25, 2015

1 check passed

CLA Commit author is a member of Elasticsearch
Details

@jasontedor jasontedor removed the review label Aug 25, 2015

@jasontedor jasontedor deleted the jasontedor:fix/13107 branch Aug 25, 2015

jasontedor added a commit that referenced this pull request Aug 25, 2015

More portable extraction of short hostname
This commit backports the fix #13109 for #13107 to 2.0 from master.
@nik9000

This comment has been minimized.

Show comment
Hide comment
@nik9000

nik9000 Aug 25, 2015

Contributor

@nik9000 I've been contemplating that as well but at a minimum it is a separate issue from this one.

Yeah, probably.

Contributor

nik9000 commented Aug 25, 2015

@nik9000 I've been contemplating that as well but at a minimum it is a separate issue from this one.

Yeah, probably.

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