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

Create with eni #1

Merged
merged 10 commits into from
Jan 24, 2017

Conversation

automaticgiant
Copy link
Owner

@automaticgiant automaticgiant commented Jan 6, 2017

here's you a pr @pielu. we still need to get both rebased onto master, then re-check.
sorry for the delay on this.

Hunter Morgan and others added 9 commits December 4, 2016 22:45
This reverts commit ef6b1555fa686dd946917f9613a8b0c91358cbb0.

no good. need to add to interface array instead
- Update Terraform’s documentation
* aws/feature/r-instance-net-iface-id: (74 commits)
  - Properly exercise network_interface_id from AWS SDK - Update Terraform’s documentation
  Update CHANGELOG.md
  provider/aws: Forces the api gateway domain name certificates to recreate the resource (hashicorp#10588)
  Update CHANGELOG.md
  provider/aws: FIxed the api_gw_domain_name replace operation (hashicorp#10179)
  Fixed note formatting
  Explicitly say `count` is not supported by modules (hashicorp#10553)
  docs/aws: Fix the discrepencies of the emr_cluster documentation (hashicorp#10578)
  Update CHANGELOG.md
  Service role is not updated on AWS for a CodeDeploy deployment group (hashicorp#9866)
  Update CHANGELOG.md
  provider/datadog hashicorp#9375: Refactor tags to a list instead of a map. (hashicorp#10570)
  Update the Vagrantfile to resolve package update/installation issue. (hashicorp#9783)
  docs/aws: Add iam_server_certificate data source to nav bar (hashicorp#10576)
  Update CHANGELOG.md
  feat/aws: add iam_server_certificate data source (hashicorp#10558)
  provider/azurerm: arm_virtual_machine panic fix
  Update .travis.yml
  provider/aws: Improved the documentation for EMR Cluster (hashicorp#10563)
  provider/azurerm: Do not pass an empty string of license_type to AMR VMs (hashicorp#10564)
  ...

# Conflicts:
#	builtin/providers/aws/resource_aws_instance.go
@automaticgiant
Copy link
Owner Author

i got mine rebased, so rebase yours @pielu, maybe fix a merge conflict, and we'll take a look.

Computed: true,
Optional: true,

Type: schema.TypeString,

Choose a reason for hiding this comment

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

According to http://docs.aws.amazon.com/cli/latest/reference/ec2/run-instances.html, you can create/launch with multiple ENIs, not just 1. So, then, this should be a TypeSet or TypeList?

Copy link
Owner Author

Choose a reason for hiding this comment

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

good question. idk anything about TypeSets and TypeLists yet. do you think that the initial implementation should focus on 1 or a list?

Choose a reason for hiding this comment

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

Why not the list? Doesn't look like a whole lot more code. Another question is if the tf config changes this value, should the instance be rebuilt? That is, should ForceNew: true

Copy link

Choose a reason for hiding this comment

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

Hi @chiradeep,
Thanks for input.

The problem with the list is that InstanceNetworkInterfaceSpecification requires network interface id and index. So Terraform template operator would have to define both at instance creation time and any change would rebuild whole instance (rather undesirable). Additionally it complicates instance resource a bit. That is why I preferred more "atomic" approach, with ability to attach eth0 of given id (versus originally hard-coded new interface) and attaching other enis explicitly (see hashicorp#10594)

Choose a reason for hiding this comment

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

OK, LGTM. I guess there are not many OS that object to hotplugging NICs after boot

@automaticgiant automaticgiant merged commit 585fe47 into automaticgiant:create_with_eni Jan 24, 2017
automaticgiant pushed a commit that referenced this pull request May 8, 2017
* added .travis.yml and deploy.sh
* added deploy script, updated travis.yml to build topic- branches
* generate random string for hostname
* plan now produces output plan, apply now consumes outputted plan
* cleanup; sane defaults
* explicit build dirs
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants