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

Address feedback about cargo make ami #1028

Merged
merged 2 commits into from
Aug 12, 2020

Conversation

zmrow
Copy link
Contributor

@zmrow zmrow commented Aug 11, 2020

commit 89e2a37afafb7d2b088572bed723971467cb8730
Author: Zac Mrowicki <[email protected]>
Date:   Tue Aug 11 17:41:16 2020 +0000

    pubsys: default AwsConfig so it's not required

    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`.
commit b69d2a218178914d8081c7e0694318da98a439ae
Author: Zac Mrowicki <[email protected]>
Date:   Tue Aug 11 17:19:42 2020 +0000

    Separate publish tasks

    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 builds only need to depend on pubsys, not buildsys.  `cargo
    make` currently rebuilds images each run.  If you want to build or 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 task 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.

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 no aws 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.

@zmrow zmrow requested review from tjkirch and bcressey August 11, 2020 17:54
@etungsten
Copy link
Contributor

etungsten commented Aug 11, 2020

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.

Can we give the same treatment to cargo make repo? The repo target currently depends on the build step.

@tjkirch
Copy link
Contributor

tjkirch commented Aug 11, 2020

Can we give the same treatment to cargo make repo? The repo target currently depends on the build step.

I think we can, yeah.

zmrow and others added 2 commits August 11, 2020 19:05
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 <[email protected]>
Co-authored-by: Tom Kirchner <[email protected]>
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 <[email protected]>
Co-authored-by: Tom Kirchner <[email protected]>
@tjkirch
Copy link
Contributor

tjkirch commented Aug 11, 2020

^ This push addresses the comment from @etungsten by making cargo make repo also check for image files internally, like AMI. We confirmed that missing image files (or migration tarball) error, and with them a repo is built correctly.

@tjkirch tjkirch requested a review from etungsten August 11, 2020 19:08
@tjkirch tjkirch merged commit 4b3f99c into bottlerocket-os:develop Aug 12, 2020
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.

4 participants