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

common: add include_deprecated option to filters #404

Merged
merged 1 commit into from
Oct 4, 2023

Conversation

lbajolet-hashicorp
Copy link
Contributor

When getting the AMIs to build a new image on, we never included deprecated AMIs, as is the default when describing images on AWS.

However, the request does offer this option, so we add this to the options to pick from when getting the base AMI.

Closes #403

@Ckarles
Copy link

Ckarles commented Jul 14, 2023

Thanks for the quick update!

Should there be an implementation on top of this to add explanatory warnings / errors which would help the maintenance and troubleshooting ?

For example:

  • By default, filter with include_deprecated.
  • If the image chosen is deprecated (.DeprecationTime < now()):
    • if not include_deprecated: return an error: "image was found for this filter, but deprecated, set include_deprecated to true"
    • if include_deprecated: display a warning: "found a deprecated image, this image will be considered valid because of include_deprecated=True"

wdyt ?

It would:

  • Clarify why it cannot find the image (it can, it's just that it's deprecated and include_deprecated is not set)
  • Give visibility that we are using deprecated images

@nywilken
Copy link
Member

If the image chosen is deprecated (.DeprecationTime < now()):
if not include_deprecated: return an error: "image was found for this filter, but deprecated, set include_deprecated to true"
if include_deprecated: display a warning: "found a deprecated image, this image will be considered valid because of include_deprecated=True"

@Ckarles this is a nice idea. But it feels a little out of scope for the filter options and data source. Especially since we would have no idea if the image is deprecated without the user actually specifying include_deprecated. The Amazon API result set will not contain the deprecated image by default if you are not the owner. So include_deprecated is need to gain visibility. If you are the own of the ami then you will see them regardless and that is where I think the deprecation_time attributed could be useful. Refer to my comment below.

  • include_deprecated to true"
  • if include_deprecated: display a warning: "found a deprecated image, this image will be considered valid because of include_deprecated=True"

wdyt ?

It would:

  • Clarify why it cannot find the image (it can, it's just that it's deprecated and include_deprecated is not set)
  • Give visibility that we are using deprecated images

I see that the data source output does not include deprecation time in the response, which is something we should look to add in a future PR. If you want to open a PR for adding that output that would allow for users to check the deprecation time within the HCL configuration template and fail the build.

I'm thinking that maybe something like the following would work

locals {
   ami_id = formatdate("YYYYMMDDhhmmss", data.amazon-ami.ubuntu.deprecation_time) > formatdate("YYYYMMDDhhmmss", timestamp()) ? data.amazon-ami.id : "deprecated ami selected change your filter"
}

source "amazon-ebs" "example" {
   source_ami = local.ami_id
}

Copy link
Member

@nywilken nywilken left a comment

Choose a reason for hiding this comment

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

I left a question, possible suggestion, but this is otherwise good to me.

builder/common/ami_filter.go Outdated Show resolved Hide resolved
builder/common/ami_filter.go Outdated Show resolved Hide resolved
When getting the AMIs to build a new image on, we never included
deprecated AMIs, as is the default when describing images on AWS.

However, the request does offer this option, so we add this to the
options to pick from when getting the base AMI.
@lbajolet-hashicorp lbajolet-hashicorp merged commit dd886db into main Oct 4, 2023
13 checks passed
@lbajolet-hashicorp lbajolet-hashicorp deleted the add_include_deprecated_option branch October 4, 2023 14:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support deprecated sources by adding a IncludeDeprecated flag to source_ami_filter
3 participants