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

builder: check Org/OU ARNs during prepare #494

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

lbajolet-hashicorp
Copy link
Contributor

When a builder accepts organisation or organisational unit ARNs as part of its config, we don't actually validate that the format matches what AWS accepts, leading to confusion among our users.

This commit checks that the provided ARNs match the expected format before running the build, and tries to provide information on what to change in the configs so they succeed later.

Related to #492

Copy link
Contributor

@JenGoldstrich JenGoldstrich left a comment

Choose a reason for hiding this comment

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

LGTM largely, and is definitely an improvement from the current experience, nice work, I made two suggestions, feel free to take them or leave them

name string
OrgARN []string
OUARN []string
expectErr bool
Copy link
Contributor

Choose a reason for hiding this comment

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

I would consider extending this to check the specific error returned, since there's several different cases that could change and these tests wouldn't fail

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm less sure about this, I think checking if the configs provoke an error when expected is more important than the contents of those errors, so I'm tempted to leave it as-is, but please feel free to push back if you think it's important

builder/common/ami_config.go Outdated Show resolved Hide resolved
When a builder accepts organisation or organisational unit ARNs as part
of its config, we don't actually validate that the format matches what
AWS accepts, leading to confusion among our users.

This commit checks that the provided ARNs match the expected format
before running the build, and tries to provide information on what to
change in the configs so they succeed later.
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.

2 participants