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: raise messages to 'warn' if AMI exists or repo doesn't #1708

Merged
merged 1 commit into from
Aug 12, 2021

Conversation

tjkirch
Copy link
Contributor

@tjkirch tjkirch commented Aug 10, 2021

Description of changes:

During development, if you make changes without committing, pubsys will
see that an AMI exists for your (-dirty) commit and not register a new
one.  Similarly, if you have a repo configured in Infra.toml and expect
to be updating it, but it doesn't exist, pubsys will create a new one.
You may not want to continue testing in these cases if you thought a new
AMI was going to be built, or an existing repo updated, with your recent
changes.  Raise these to 'warn' level so they're more obvious.

Testing done:

First cargo make ami OK:

21:26:19 [INFO] Registered AMI 'bottlerocket-aws-k8s-1.21-x86_64-v1.2.0-74af19f7' in us-west-2: ami-001e3...

Second cargo make ami WARN:

21:26:55 [WARN] Found 'bottlerocket-aws-k8s-1.21-x86_64-v1.2.0-74af19f7' already registered in us-west-2: ami-001e3...

If a configured repo doesn't exist to update in cargo make repo:

16:54:01 [WARN] Did not find repo at 'file:///home/me/work/bottlerocket/build/repos/default/latest/aws-k8s-1.21/x86_64', starting a new one

If it does exist:

16:57:22 [INFO] Found metadata and target URLs, loading existing repository

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.

@bcressey
Copy link
Contributor

Thanks! I run into this pretty often when rebuilding without a new commit.

The other thing that gets me is when a repo doesn't exist, but it's actually supposed to be there and I've used aws s3 sync without making the artifacts public.

It's easy to end up building what I intend to be two or three updates for testing purposes, but the final repo only has one because pubsys isn't able to fetch existing content to merge.

@tjkirch
Copy link
Contributor Author

tjkirch commented Aug 11, 2021

The other thing that gets me is when a repo doesn't exist, but it's actually supposed to be there and I've used aws s3 sync without making the artifacts public.

That makes sense, I'll look at making that a warning in this PR as well.

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.

💎

@webern
Copy link
Contributor

webern commented Aug 11, 2021

It doesn't block this but I was actually thinking that pubsys should error out when the AMI name exists and have an --overwrite flag. Our Makefile.toml could offer a PUBSYS_OVERWRITE option to exercise that flag. In the past, not-overwriting an existing AMI has been costly in test scenarios. Now I name every AMI with a UUID.

During development, if you make changes without committing, pubsys will
see that an AMI exists for your (-dirty) commit and not register a new
one.  Similarly, if you have a repo configured in Infra.toml and expect
to be updating it, but it doesn't exist, pubsys will create a new one.
You may not want to continue testing in these cases if you thought a new
AMI was going to be built, or an existing repo updated, with your recent
changes.  Raise these to 'warn' level so they're more obvious.
@tjkirch tjkirch changed the title pubsys: raise message to warn level if AMI exists pubsys: raise messages to 'warn' if AMI exists or repo doesn't Aug 11, 2021
@tjkirch
Copy link
Contributor Author

tjkirch commented Aug 11, 2021

^ This push also warns if a repo you're probably trying to update (because you have it configured in Infra.toml) doesn't exist. Updated testing.

thinking that pubsys should error out when the AMI name exists and have an --overwrite flag

I'm sympathetic to this, particularly because we already fail if the repo target directory exists, but I think we should probably consider it separately.

@tjkirch tjkirch merged commit 5e427f5 into bottlerocket-os:develop Aug 12, 2021
@tjkirch tjkirch deleted the existing-ami branch August 12, 2021 16:45
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