-
Notifications
You must be signed in to change notification settings - Fork 511
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
Address feedback about cargo make ami
#1028
Conversation
Can we give the same treatment to |
I think we can, yeah. |
If you just want to build images, you don't need to build the pubsys tool first, only buildsys. This change splits the tasks so that builds only need to depend on buildsys. Similarly, AMI and repo builds only need to depend on pubsys, not buildsys. `cargo make` currently rebuilds images each run. If you want to build repos or build/copy AMIs based on a previous image build, you don't want to wait for image rebuilds, and you don't want to use different files - you want to use the same image files you had. This change makes the ami and repo tasks depend only on pubsys and sources, and checks for image file existence internally. (The image file names include version and commit, so you can't accidentally use an old build.) This will also apply for `cargo make ssm`, where timing is even more critical. Co-authored-by: Zac Mrowicki <mrowicki@amazon.com> Co-authored-by: Tom Kirchner <tjk@amazon.com>
Have serde default `aws.region` so the user does not have to have an empty `aws.region` table if they have no region-specific configuration. Have serde default `aws.regions` in case the user only wants to specify regions on the command line with `PUBLISH_REGIONS`. Derive Default on AwsConfig now that all sections of `aws` in Infra.toml are optional, so the user doesn't need an empty `[aws]` section if they intend to use default credential mechanisms and specify `PUBLISH_REGIONS`. Co-authored-by: Zac Mrowicki <mrowicki@amazon.com> Co-authored-by: Tom Kirchner <tjk@amazon.com>
^ This push addresses the comment from @etungsten by making |
Testing done:
Before:
cargo make
built pubsys unnecessarily.After:
cargo make
does not build pubsys;cargo make ami
still does.Before: images rebuilt every time before
cargo make ami
when it should use existing images.After:
cargo make ami
fails appropriately if no images exist, and runs quickly and correctly when they do.Before: aws.region was required in Infra.toml even when you don't have region-specific config.
Before: aws.regions was required in Infra.toml even if you wanted to override with PUBLISH_REGIONS.
After:
cargo make ami -e PUBLISH_REGIONS=bla1,bla2
works, even with noaws
section in Infra.toml.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.