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

pubsys: add AMI registration/copying #1010

Merged
merged 2 commits into from
Aug 10, 2020
Merged

Conversation

tjkirch
Copy link
Contributor

@tjkirch tjkirch commented Aug 6, 2020

Description of changes:

commit fdffc56bb4a7eaae368d16b5ca974c65b0063dcc
Author: Zac Mrowicki <[email protected]>
Date:   Thu Jul 30 23:30:39 2020 +0000

    Add the "ami" subcommand to pubsys

    This lets you register an AMI from the latest build and copy it to a list of
    regions.

commit 659fe384c7f2990294080f712efbb28a4aaf2fcd
Author: Zac Mrowicki <[email protected]>
Date:   Tue Jul 28 23:25:43 2020 +0000

    Make all fields of Infra.toml optional

    Pubsys subcommands do not use the same parts of Infra.toml, so they have
    to be optional if we want to have a common config file.  The subcommands
    will confirm that the necessary fields are present.

Infra.toml.example was updated with the configuration needed for building AMIs; only a region list is necessary, and that can be overridden by specifying -e PUBLISH_REGIONS if desired. You can also override the AMI name and volume sizes, if desired.

Testing done:

Used cargo make ami to register single AMIs, register/copy AMIs in multiple regions, register with global and regional roles. A registered Bottlerocket aws-k8s-1.17 AMI ran/connected OK. Also reran cargo make repo to ensure existing functionality wasn't affected by config change.

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.

@tjkirch
Copy link
Contributor Author

tjkirch commented Aug 6, 2020

^ This push fixes the coldsnap dependency in Cargo.toml to actually use it from crates.io.

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.

🦓

Makefile.toml Outdated Show resolved Hide resolved
tools/pubsys/src/aws/ami/wait.rs Outdated Show resolved Hide resolved
tools/pubsys/Infra.toml.example Outdated Show resolved Hide resolved
Makefile.toml Outdated Show resolved Hide resolved
tools/pubsys/src/aws/ami/mod.rs Outdated Show resolved Hide resolved
tools/pubsys/src/aws/ami/register.rs Show resolved Hide resolved
@bcressey
Copy link
Contributor

bcressey commented Aug 7, 2020

Also this is amazing, nice work!

@tjkirch
Copy link
Contributor Author

tjkirch commented Aug 7, 2020

I'm not able to push the change until @zmrow's back, but this addresses @bcressey's feedback above: https://gist.github.com/tjkirch/b0ac076be189af44551a20f5c5bed7c5 Still able to make AMIs OK.

@tjkirch
Copy link
Contributor Author

tjkirch commented Aug 10, 2020

^ This push addresses @bcressey's comments above, as in my gist above.

@tjkirch
Copy link
Contributor Author

tjkirch commented Aug 10, 2020

^ This push is just a rebase on develop to fix conflicts.

Pubsys subcommands do not use the same parts of Infra.toml, so they have
to be optional if we want to have a common config file.  The subcommands
will confirm that the necessary fields are present.
@zmrow
Copy link
Contributor

zmrow commented Aug 10, 2020

^ This push is just a rebase on develop to fix CI.

This lets you register an AMI from the latest build and copy it to a list of
regions.

Co-authored-by: Zac Mrowicki <[email protected]>
Co-authored-by: Tom Kirchner <[email protected]>
@tjkirch
Copy link
Contributor Author

tjkirch commented Aug 10, 2020

^ This push moves PUBLISH_AMI_NAME to the [env.development] section of Makefile.toml, which is necessary so that when you change BUILDSYS_ARCH it can actually insert the arch into the AMI name. Unfortunately this means it can't be overridden, for the same reasons we have trouble with other layered variables in Makefile.toml; we should find a more comprehensive solution for that.

@tjkirch tjkirch requested a review from bcressey August 10, 2020 22:53
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.

Nice!

tools/pubsys/src/repo.rs Show resolved Hide resolved
tools/pubsys/Infra.toml.example Show resolved Hide resolved
tools/pubsys/src/aws/ami/mod.rs Show resolved Hide resolved
@tjkirch tjkirch merged commit aee7cd7 into bottlerocket-os:develop Aug 10, 2020
@zmrow zmrow deleted the pubsys-ami branch August 11, 2020 14:21
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