Skip to content

cosalib/aws: check for 'amis' key in buildmeta#1299

Merged
openshift-merge-robot merged 1 commit intocoreos:masterfrom
miabbott:ami_key_error
Apr 3, 2020
Merged

cosalib/aws: check for 'amis' key in buildmeta#1299
openshift-merge-robot merged 1 commit intocoreos:masterfrom
miabbott:ami_key_error

Conversation

@miabbott
Copy link
Copy Markdown
Member

Noticed this when I did cosa buildextend-aws (without --upload)
and then tried to do cosa aws-replicate. Without this change, it
would blow up with a KeyError since the AMIs were never uploaded.

Comment thread src/cosalib/aws.py Outdated
Copy link
Copy Markdown
Contributor

@bgilbert bgilbert left a comment

Choose a reason for hiding this comment

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

LGTM

@cgwalters
Copy link
Copy Markdown
Member

/lgtm

@miabbott
Copy link
Copy Markdown
Member Author

miabbott commented Apr 1, 2020

Rebased to pull in #1303

@darkmuggle darkmuggle added lgtm and removed lgtm labels Apr 2, 2020
Comment thread src/cosalib/aws.py Outdated
build.refresh_meta()
buildmeta = build.meta
if len(buildmeta['amis']) < 1:
if 'amis' not in buildmeta:
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hmm, I think we do want to check that there's at least one source AMI though, no? I mean, we'll error out anyway later on at:

        args.source_region = buildmeta['amis'][0]['name']

But the error message here is nicer than what IndexError will print. :) So maybe just:

if len(buildmeta.get('amis', [])) < 1:

?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I'll go this path, but I wonder how someone would end up with an empty amis list?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Ah nevermind, I see why checking length makes sense now.

Comment thread src/cosalib/aws.py Outdated
if len(buildmeta['amis']) < 1:
if 'amis' not in buildmeta:
raise SystemExit(("buildmeta doesn't contain source AMIs."
" Run buildextend-aws first"))
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

And maybe here "Run buildextend-aws --upload first." to be more accurate.

@jlebon
Copy link
Copy Markdown
Member

jlebon commented Apr 2, 2020

Sorry, didn't mean to also approve. (I started using https://github.com/sindresorhus/refined-github in my Firefox and it's really good for the most part, but still getting used to some of the changes.)

Noticed this when I did `cosa buildextend-aws` (without `--upload`)
and then tried to do `cosa aws-replicate`.  Without this change, it
would blow up with a KeyError since the AMIs were never uploaded.
@jlebon
Copy link
Copy Markdown
Member

jlebon commented Apr 3, 2020

/lgtm

@openshift-ci-robot
Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bgilbert, cgwalters, jlebon, miabbott

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:
  • OWNERS [bgilbert,cgwalters,jlebon,miabbott]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-merge-robot openshift-merge-robot merged commit 78ffdd0 into coreos:master Apr 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants