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

aws_ami: Add root_snapshot_id attribute #1572

Merged
merged 2 commits into from
Oct 10, 2017
Merged

Conversation

phinze
Copy link
Contributor

@phinze phinze commented Sep 1, 2017

Current limitations in HCL prevent it from being possible to reference
an AMI's root snapshot ID via its ID. This is because the snapshot ID is
deeply nested in the block_device_mappings set. HCL is not
yet flexible enough to allow a lookup expression to extract the snapshot
ID embedded within that structure.

This makes it impossible to use resources like
aws_snapshot_create_volume_permission without manually doing the AMI
ID -> Snapshot ID translation outside of Terraform.

Here, we promote the Snapshot ID of the root volume to a top-level
root_snapshot_id attribute for AMI data sources and resources.

This field will be blank for instance store AMIs or for weird conditions
where there's no snapshot ID recorded (See commit message here:
bed5f62), but it enables usage of the
resource referenced above for the majority of scenarios.

Current limitations in HCL prevent it from being possible to reference
an AMI's root snapshot ID via its ID. This is because the snapshot ID is
deeply nested in the `block_device_mappings` set. HCL is not
yet flexible enough to allow a lookup expression to extract the snapshot
ID embedded within that structure.

This makes it impossible to use resources like
`aws_snapshot_create_volume_permission` without manually doing the AMI
ID -> Snapshot ID translation outside of Terraform.

Here, we promote the Snapshot ID of the root volume to a top-level
`root_snapshot_id` attribute for AMI data sources and resources.

This field will be blank for instance store AMIs or for weird conditions
where there's no snapshot ID recorded (See commit message here:
bed5f62), but it enables usage of the
resource referenced above for the majority of scenarios.
@phinze phinze added the enhancement Requests to existing resources that expand the functionality or scope. label Sep 1, 2017
@phinze
Copy link
Contributor Author

phinze commented Sep 1, 2017

❯ go test -run 'TestAccAWSAmiDataSource_*' -v .
=== RUN   TestAccAWSAmiDataSource_natInstance
--- PASS: TestAccAWSAmiDataSource_natInstance (17.32s)
=== RUN   TestAccAWSAmiDataSource_windowsInstance
--- PASS: TestAccAWSAmiDataSource_windowsInstance (14.51s)
=== RUN   TestAccAWSAmiDataSource_instanceStore
--- PASS: TestAccAWSAmiDataSource_instanceStore (11.44s)
=== RUN   TestAccAWSAmiDataSource_owners
--- PASS: TestAccAWSAmiDataSource_owners (214.44s)
=== RUN   TestAccAWSAmiDataSource_ownersEmpty
--- PASS: TestAccAWSAmiDataSource_ownersEmpty (105.74s)
=== RUN   TestAccAWSAmiDataSource_localNameFilter
--- PASS: TestAccAWSAmiDataSource_localNameFilter (140.15s)
PASS
ok      github.com/terraform-providers/terraform-provider-aws/aws       503.864s
=== RUN   TestAccAWSAMI_basic
--- PASS: TestAccAWSAMI_basic (52.48s)
=== RUN   TestAccAWSAMI_snapshotSize
--- PASS: TestAccAWSAMI_snapshotSize (51.43s)

/cc @apparentlymart as we discussed this a bit

Copy link
Member

@radeksimko radeksimko left a comment

Choose a reason for hiding this comment

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

Besides 2 nitpicks this LGTM.

size = 8
tags {
Name = "testAccAmiConfig_basic"
}
Copy link
Member

Choose a reason for hiding this comment

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

Nitpick - I think the convention is 2 spaces per indentation.

size = 20
tags {
Name = "testAccAmiConfig_snapshotSize"
}
Copy link
Member

Choose a reason for hiding this comment

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

^

@radeksimko
Copy link
Member

I'm not sure why did I point out the correct indentation as incorrect 🤔
Anyways - I just resolved conflicts... 🚢

Thanks for the PR. 👍

@ghost
Copy link

ghost commented Apr 10, 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 feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. Thanks!

@ghost ghost locked and limited conversation to collaborators Apr 10, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement Requests to existing resources that expand the functionality or scope.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants