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

Allow specifying AMI name with PUBLISH_AMI_NAME #1091

Merged
merged 1 commit into from
Aug 31, 2020

Conversation

tjkirch
Copy link
Contributor

@tjkirch tjkirch commented Aug 28, 2020

AMI name wasn't overridable because it was in the env.development section of
the Makefile, since it depends on variables from the env section.

This change sets a DEFAULT version of the variable to the fixed name we had
before, and uses it if the user didn't set PUBLISH_AMI_NAME.

Testing done:

cargo make ami produced an AMI with the existing name/description format. cargo make ami -e PUBLISH_AMI_NAME=foo produced an AMI named foo.

Terms of contribution:

By submitting this pull request, I agree that this contribution is dual-licensed under the terms of both the Apache License, version 2.0, and the MIT license.

AMI name wasn't overridable because it was in the env.development section of
the Makefile, since it depends on variables from the env section.

This change sets a DEFAULT version of the variable to the fixed name we had
before, and uses it if the user didn't set PUBLISH_AMI_NAME.
Copy link
Contributor

@webern webern left a comment

Choose a reason for hiding this comment

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

👍

@bcressey
Copy link
Contributor

What's the expected use case for this? Since AMI names have to be unique, this feels pretty restrictive unless there are external tools that duplicate the logic in Makefile.toml to arrive at a version specifier.

It might be better to support a template in Infra.toml or in a separate file, like we do for SSM parameters.

@tjkirch
Copy link
Contributor Author

tjkirch commented Aug 30, 2020

What's the expected use case for this? Since AMI names have to be unique, this feels pretty restrictive unless there are external tools that duplicate the logic in Makefile.toml to arrive at a version specifier.

It might be better to support a template in Infra.toml or in a separate file, like we do for SSM parameters.

It's not used in the way you'd use a template, it's used by developers (like Sam and I) who like to name test AMIs differently, not related to criteria that could easily be put into a template. For example, I name mine with a timestamp format I like, because I could build several with the same commit.

I agree that a template could be useful to handle the "normal" case of naming AMIs based on criteria known to the build system, rather than the hardcoded template in the makefile, but I think that's a separate improvement. If you don't like this change, I could get away with a template if we put in a bit more expressivity than normal so they can be unique.

@bcressey
Copy link
Contributor

I agree that a template could be useful to handle the "normal" case of naming AMIs based on criteria known to the build system, rather than the hardcoded template in the makefile, but I think that's a separate improvement. If you don't like this change, I could get away with a template if we put in a bit more expressivity than normal so they can be unique.

This is fine. Maybe open an issue to capture thoughts on template support, to attract the attention of anyone looking for more customization.

Copy link
Contributor

@zmrow zmrow left a comment

Choose a reason for hiding this comment

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

👔

Copy link
Contributor

@samuelkarp samuelkarp left a comment

Choose a reason for hiding this comment

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

Thanks @tjkirch!

@tjkirch tjkirch merged commit 0fa355c into bottlerocket-os:develop Aug 31, 2020
@tjkirch tjkirch deleted the ami-name-var branch August 31, 2020 20:33
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.

5 participants