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

Sort the list of private_ips #6986

Closed
wants to merge 1 commit into from

Conversation

freimer
Copy link

@freimer freimer commented Jun 2, 2016

Very simple change to sort the private_ips before tagging the first one as the primary. This should really be changed to have private_ips as a sorted list, and use the first, or better, use private_ip as the primary IP address for the instance and private_ips as secondary IP addresses. For now, this appears to work. Ran all tests, and created all binaries. Tested binary on Mac OS X and it uses the "lowest" IP address in the list. Note that this is a string sort, so lowest is dependent on the natural / canonical order of strings, not numbers, and certainly not real IP addresses. This works for me, and can't possibly cause problems over a completely arbitrary ordering that was present in the original, but should really sort IPs properly (or use private_ip and private_ips as described above, then no sorting required).

@Joe6547654
Copy link

Hi. I am using terraform to create an instance with multiple ips in AWS.
I see your comments. I tried setting with private_ip for the primary ip and private_ips for secondary ips. However, I found out that the primary ip does not get listed when the instance is created.
Is there a proper way to set the primary and secondary ips in terraform?
Thank you.

@justinclayton
Copy link
Contributor

What needs to happen to get this merged?

@apparentlymart
Copy link
Contributor

This PR was opened when the AWS provider was (and all the other providers were) in this single repository; it no longer applies because the AWS provider now has its own repository. Unfortunately a number of PRs got left behind in this move because there is no good automated way to move them. 😖

Before this could be merged it would need to be re-applied against the equivalent code in the separate repository. Ideally it would also be accompanied by an acceptance test, though that may be difficult if the API is returning the list in an unpredictable order since the order might be right sometimes just by coincidence.

@apparentlymart
Copy link
Contributor

Hello again @freimer, and thanks for working on this!

As part of the the Terraform 0.10 release earlier this year, all of the Terraform providers were moved to their own repositories in the terraform-providers GitHub organization, and removed from the Terraform Core repository.

Unfortunately due to the fact that new issues and pull requests are being opened constantly, it was not possible for the various provider maintainers to merge all outstanding pull requests before this split, and there is no automatic way to migrate a pull request to a new repository.

As a result, this pull request can sadly no longer be applied as-is, and so I'm going to close it.

If you or someone else has the time and motivation to apply same changes to the aws provider repository and open a new PR there, the maintainers of that provider should be able to review and merge it.

Thanks again for working on this, and sorry it was not able to be merged before the provider repository changes.

@ghost
Copy link

ghost commented Apr 6, 2020

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@ghost ghost locked and limited conversation to collaborators Apr 6, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants