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

New resource: aws_ami_description - lookup AMI via DescribeImages #4396

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
6 participants
@vancluever
Contributor

vancluever commented Dec 19, 2015

I made this resource because I wanted an easy way to look up the latest AWS NAT images. Of course, with the advent of the new NAT gateway and the respective resource in Terraform (which I just found out about today!), this is a bit moot, but I'm hoping this will find other use cases. I myself will be using this to simplify some of my build pipelines that require a pre-built instance, produced by things like packer.

Check it out, any questions or comments let me know. Docs are included in this PR, of course. Tests cover looking up a NAT instance, a Windows instance, and a Ubuntu instance store instance.

@antonbabenko

This comment has been minimized.

Show comment
Hide comment
@antonbabenko

antonbabenko Dec 20, 2015

Contributor

👍 for this kind of resource, but I have some suggestions. I think aws_ami_find is better name, also argument references should be more like in Ansible ec2_ami_find module (http://docs.ansible.com/ansible/ec2_ami_find_module.html) and compatible with existing ami resource (https://www.terraform.io/docs/providers/aws/r/ami.html). More specifically, virtualization_type, architecture, name, ami_tags, is_public, owner, region, no_result_action.

Contributor

antonbabenko commented Dec 20, 2015

👍 for this kind of resource, but I have some suggestions. I think aws_ami_find is better name, also argument references should be more like in Ansible ec2_ami_find module (http://docs.ansible.com/ansible/ec2_ami_find_module.html) and compatible with existing ami resource (https://www.terraform.io/docs/providers/aws/r/ami.html). More specifically, virtualization_type, architecture, name, ami_tags, is_public, owner, region, no_result_action.

@vancluever

This comment has been minimized.

Show comment
Hide comment
@vancluever

vancluever Dec 20, 2015

Contributor

@antonbabenko, sorry but I disagree. The name is fine, I don't see the need to change it, same with the parameters. In fact both the resource name and filter parameter are modelled after the API call it consumes.

To address the issue of why not have more finer grained parameters:

  • filter parameters can be used with most if not all of the flags above, and multiples are allowed. The submitted docs refer the reader to the CLI docs where they can get a full reference of allowed filter keys - this also ensures that any changes/updates to these keys do not require an update the the source. The only one that I see missing is region but I think that is because Ansible needs to make the EC2 connection when it's configuring the resource, while Terraform handles this on the provider level. It is not a filter parameter.
  • aws_ami uses a different API call that requires a lot of the parameters mentioned. Further, aws_ami creates images, and does not search for them.
  • A conscious decision was made to fail if the result was <> 1 because the intention here is to ensure that a a single AMI is pulled per definition, allowing one to rely on the result that is returned. no_result_action is unnecessary here as we already know what the action will be if there is no result, and that is to fail. Again the docs say as much.
Contributor

vancluever commented Dec 20, 2015

@antonbabenko, sorry but I disagree. The name is fine, I don't see the need to change it, same with the parameters. In fact both the resource name and filter parameter are modelled after the API call it consumes.

To address the issue of why not have more finer grained parameters:

  • filter parameters can be used with most if not all of the flags above, and multiples are allowed. The submitted docs refer the reader to the CLI docs where they can get a full reference of allowed filter keys - this also ensures that any changes/updates to these keys do not require an update the the source. The only one that I see missing is region but I think that is because Ansible needs to make the EC2 connection when it's configuring the resource, while Terraform handles this on the provider level. It is not a filter parameter.
  • aws_ami uses a different API call that requires a lot of the parameters mentioned. Further, aws_ami creates images, and does not search for them.
  • A conscious decision was made to fail if the result was <> 1 because the intention here is to ensure that a a single AMI is pulled per definition, allowing one to rely on the result that is returned. no_result_action is unnecessary here as we already know what the action will be if there is no result, and that is to fail. Again the docs say as much.
@mtb-xt

This comment has been minimized.

Show comment
Hide comment
@mtb-xt

mtb-xt Dec 20, 2015

👍 This would really make life easier for me!

mtb-xt commented Dec 20, 2015

👍 This would really make life easier for me!

@apparentlymart

This comment has been minimized.

Show comment
Hide comment
@apparentlymart

apparentlymart Dec 21, 2015

Contributor

Thanks for this, @vancluever.

Over in #4169 we have been discussing a new concept for fetching data in Terraform, with finding AMIs being one example use-case. That would allow this result to be achieved in a more predictable way.

That idea is still under discussion, so not necessarily a reason to block progress on this simpler change but note that the existing limitations with read-only resources described in that issue would apply to this AMI resource too.

Contributor

apparentlymart commented Dec 21, 2015

Thanks for this, @vancluever.

Over in #4169 we have been discussing a new concept for fetching data in Terraform, with finding AMIs being one example use-case. That would allow this result to be achieved in a more predictable way.

That idea is still under discussion, so not necessarily a reason to block progress on this simpler change but note that the existing limitations with read-only resources described in that issue would apply to this AMI resource too.

@vancluever

This comment has been minimized.

Show comment
Hide comment
@vancluever

vancluever Dec 21, 2015

Contributor

Thanks @apparentlymart, I have some thoughts I'll post over there too, also I'd love for this to be a pilot for that! I'm not too sure how far out it is, but once it's hashed out I'd be more than happy to do the work to make this a data source.

In the meantime if there's anything else needed to get this merged let me know.

Contributor

vancluever commented Dec 21, 2015

Thanks @apparentlymart, I have some thoughts I'll post over there too, also I'd love for this to be a pilot for that! I'm not too sure how far out it is, but once it's hashed out I'd be more than happy to do the work to make this a data source.

In the meantime if there's anything else needed to get this merged let me know.

@vancluever

This comment has been minimized.

Show comment
Hide comment
@vancluever

vancluever Dec 22, 2015

Contributor

Updated the PR with the executable_users and owners parameters, this allows further filtering based on API/SDK parameters and more usefully allows one to search on their own images (using the self keyword).

executable_users is a little tough to test as the test will fail if you don't have any images, and otherwise you are searching on arbitrary account IDs. I did have a test in for it before pushing, but it was failing due to no results, so at least it's working!

Also made a few corrections to the docs (filter values were not lists when they should have been).

Contributor

vancluever commented Dec 22, 2015

Updated the PR with the executable_users and owners parameters, this allows further filtering based on API/SDK parameters and more usefully allows one to search on their own images (using the self keyword).

executable_users is a little tough to test as the test will fail if you don't have any images, and otherwise you are searching on arbitrary account IDs. I did have a test in for it before pushing, but it was failing due to no results, so at least it's working!

Also made a few corrections to the docs (filter values were not lists when they should have been).

@vancluever

This comment has been minimized.

Show comment
Hide comment
@vancluever

vancluever Dec 23, 2015

Contributor

Another update - ensure a missing Ebs section of BlockDeviceMappings is handled gracefully, and also a minor correction to the docs.

Contributor

vancluever commented Dec 23, 2015

Another update - ensure a missing Ebs section of BlockDeviceMappings is handled gracefully, and also a minor correction to the docs.

@ketzacoatl

This comment has been minimized.

Show comment
Hide comment
@ketzacoatl

ketzacoatl Jan 15, 2016

Contributor

I would be interested to know - is there anything blocking this PR as it stands?

Contributor

ketzacoatl commented Jan 15, 2016

I would be interested to know - is there anything blocking this PR as it stands?

@vancluever

This comment has been minimized.

Show comment
Hide comment
@vancluever

vancluever Jan 15, 2016

Contributor

Hey @ketzacoatl yeah I'm not too sure. Could be the HashiCorp folks are just backed up on other stuff.

But yeah guys if there's anything that needs that is holding it up let me know. I have been using this in production now with some pretty good results and it's a part of one of my pipelines now, and soon to be part of a second.

Contributor

vancluever commented Jan 15, 2016

Hey @ketzacoatl yeah I'm not too sure. Could be the HashiCorp folks are just backed up on other stuff.

But yeah guys if there's anything that needs that is holding it up let me know. I have been using this in production now with some pretty good results and it's a part of one of my pipelines now, and soon to be part of a second.

@mtb-xt

This comment has been minimized.

Show comment
Hide comment
@mtb-xt

mtb-xt Feb 22, 2016

Can this be merged? Maybe @phinze , please?

mtb-xt commented Feb 22, 2016

Can this be merged? Maybe @phinze , please?

@vancluever

This comment has been minimized.

Show comment
Hide comment
@vancluever

vancluever Apr 7, 2016

Contributor

Everyone - it's been a while since this PR is in, and I'm still using it, so I figured I'd give it a rebase to make it easier to merge.

Would love to see this in upstream - even it it gets removed in 0.7 (and again, I would be totally fine with porting it over to a data source when that happens).

Contributor

vancluever commented Apr 7, 2016

Everyone - it's been a while since this PR is in, and I'm still using it, so I figured I'd give it a rebase to make it easier to merge.

Would love to see this in upstream - even it it gets removed in 0.7 (and again, I would be totally fine with porting it over to a data source when that happens).

@vancluever vancluever closed this Apr 20, 2016

@vancluever vancluever deleted the paybyphone:paybyphone_aws_ami_description branch Apr 20, 2016

@vancluever vancluever restored the paybyphone:paybyphone_aws_ami_description branch Apr 20, 2016

@apparentlymart

This comment has been minimized.

Show comment
Hide comment
@apparentlymart

apparentlymart Apr 20, 2016

Contributor

@vancluever I was planning to use aws_ami as a first data source to spin the wheels of that feature a bit and develop some general patterns for mapping AWS Describe* functions to data sources... my intent was to take the guts of your implementation here and use it as my starting point, if that's okay with you...

In which case, I expect we'd merge the data source equivalent of this as part of the data source work, as an end-to-end proof that it's working.

Contributor

apparentlymart commented Apr 20, 2016

@vancluever I was planning to use aws_ami as a first data source to spin the wheels of that feature a bit and develop some general patterns for mapping AWS Describe* functions to data sources... my intent was to take the guts of your implementation here and use it as my starting point, if that's okay with you...

In which case, I expect we'd merge the data source equivalent of this as part of the data source work, as an end-to-end proof that it's working.

@vancluever

This comment has been minimized.

Show comment
Hide comment
@vancluever

vancluever Apr 20, 2016

Contributor

Hey @apparentlymart, I was just typing a message! lol.

I actually fat-fingered the delete button on this branch while doing some cleanup on our fork. Is there any way to restore the status on this PR? Whatever works. :)

Contributor

vancluever commented Apr 20, 2016

Hey @apparentlymart, I was just typing a message! lol.

I actually fat-fingered the delete button on this branch while doing some cleanup on our fork. Is there any way to restore the status on this PR? Whatever works. :)

@apparentlymart

This comment has been minimized.

Show comment
Hide comment
@apparentlymart

apparentlymart Apr 20, 2016

Contributor

I'll reopen it for now and if I do end up merging something equivalent to this in the data sources work I'll come back and update it.

Contributor

apparentlymart commented Apr 20, 2016

I'll reopen it for now and if I do end up merging something equivalent to this in the data sources work I'll come back and update it.

@vancluever

This comment has been minimized.

Show comment
Hide comment
@vancluever

vancluever Apr 20, 2016

Contributor

Thanks @apparentlymart! And yeah, it's my hope that the work here and in #4848 would find its way into #4961. Cheers!

Contributor

vancluever commented Apr 20, 2016

Thanks @apparentlymart! And yeah, it's my hope that the work here and in #4848 would find its way into #4961. Cheers!

@vancluever

This comment has been minimized.

Show comment
Hide comment
@vancluever

vancluever May 23, 2016

Contributor

Hey @apparentlymart, just wondering if you started work on this as a data source yet? If not, I can get it done.

Contributor

vancluever commented May 23, 2016

Hey @apparentlymart, just wondering if you started work on this as a data source yet? If not, I can get it done.

@apparentlymart

This comment has been minimized.

Show comment
Hide comment
@apparentlymart

apparentlymart May 23, 2016

Contributor

@vancluever I have not worked directly on this yet but in #6819 I have a general utility ec2_filters.go that does a bunch of the work for these EC2-related data sources... you might want to use that in your work here.

I also have these notes on the design methodology I was following for these, in ther hope of keeping them relatively consistent with each other: https://gist.github.com/apparentlymart/e62c81db609eca1fe08dc8226dd48485

Contributor

apparentlymart commented May 23, 2016

@vancluever I have not worked directly on this yet but in #6819 I have a general utility ec2_filters.go that does a bunch of the work for these EC2-related data sources... you might want to use that in your work here.

I also have these notes on the design methodology I was following for these, in ther hope of keeping them relatively consistent with each other: https://gist.github.com/apparentlymart/e62c81db609eca1fe08dc8226dd48485

@vancluever

This comment has been minimized.

Show comment
Hide comment
@vancluever

vancluever May 25, 2016

Contributor

@apparentlymart roger, I'll start work on it then! I'll take a look at that, sounds like based on name alone there might be some helpers there that I had to write for this as well, so I'll make sure to re-factor to use those.

Contributor

vancluever commented May 25, 2016

@apparentlymart roger, I'll start work on it then! I'll take a look at that, sounds like based on name alone there might be some helpers there that I had to write for this as well, so I'll make sure to re-factor to use those.

@vancluever vancluever referenced this pull request May 25, 2016

Closed

Search for AWS AMIs #6862

vancluever pushed a commit to paybyphone/terraform that referenced this pull request May 28, 2016

Chris Marchesi
provider/aws: New data source: aws_ami_description
This data source allows one to look up the most recent AMI for a specific
set of parameters, much like aws ec2 describe-images in the AWS CLI.

Basically a refresh of hashicorp/terraform#4396, in data source form.

vancluever pushed a commit to paybyphone/terraform that referenced this pull request May 28, 2016

Chris Marchesi
provider/aws: New data source: aws_ami_description
This data source allows one to look up the most recent AMI for a specific
set of parameters, much like aws ec2 describe-images in the AWS CLI.

Basically a refresh of hashicorp/terraform#4396, in data source form.

vancluever added a commit to paybyphone/terraform that referenced this pull request May 28, 2016

provider/aws: New data source: aws_ami_description
This data source allows one to look up the most recent AMI for a specific
set of parameters, much like aws ec2 describe-images in the AWS CLI.

Basically a refresh of hashicorp/terraform#4396, in data source form.

vancluever pushed a commit to paybyphone/terraform that referenced this pull request May 29, 2016

Chris Marchesi
provider/aws: New data source: aws_ami
This data source allows one to look up the most recent AMI for a specific
set of parameters, much like aws ec2 describe-images in the AWS CLI.

Basically a refresh of hashicorp/terraform#4396, in data source form.
@vancluever

This comment has been minimized.

Show comment
Hide comment
@vancluever

vancluever May 29, 2016

Contributor

Now that #6911 is merged, closing this one off as we now have this in data source form with aws_ami!

Contributor

vancluever commented May 29, 2016

Now that #6911 is merged, closing this one off as we now have this in data source form with aws_ami!

@vancluever vancluever closed this May 29, 2016

@vancluever vancluever deleted the paybyphone:paybyphone_aws_ami_description branch May 30, 2016

grubernaut pushed a commit to terraform-providers/terraform-provider-aws that referenced this pull request Jun 9, 2017

Chris Marchesi
provider/aws: New data source: aws_ami
This data source allows one to look up the most recent AMI for a specific
set of parameters, much like aws ec2 describe-images in the AWS CLI.

Basically a refresh of hashicorp/terraform#4396, in data source form.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment