-
Notifications
You must be signed in to change notification settings - Fork 9.7k
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
provider/aws: Add key_name_prefix argument to aws_key_pair resource #9993
provider/aws: Add key_name_prefix argument to aws_key_pair resource #9993
Conversation
…pair-name_prefix # Conflicts: # website/source/docs/providers/aws/r/key_pair.html.markdown
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking good - one question within
It seems like that acceptance tests didn't run here - can you check them again and drop the _key suffix?
P.
value := v.(string) | ||
if len(value) > 100 { | ||
errors = append(errors, fmt.Errorf( | ||
"%q cannot be longer than 100 characters, name is limited to 255", k)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason why you chose 100 as the max length here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it :)
This LGTM! Thanks for the work on this :)
|
…9993) * Added key_name_prefix to aws_key_pair resource schema. * Added logic to prefix the aws_key_pair name on create. * Added aws_key_pair test config for key_name_prefix case. * Copied test cases from testAccAWSSecurityGroup namespace. * Modified copied test case to suit aws_key_pair resource. * Changed required flag to optional on key_name argument for aws_key_pair resource. * Added documentation for key_name_prefix argument. * Code style fix. * Fixed undefined variable error in test.
…ashicorp#9993) * Added key_name_prefix to aws_key_pair resource schema. * Added logic to prefix the aws_key_pair name on create. * Added aws_key_pair test config for key_name_prefix case. * Copied test cases from testAccAWSSecurityGroup namespace. * Modified copied test case to suit aws_key_pair resource. * Changed required flag to optional on key_name argument for aws_key_pair resource. * Added documentation for key_name_prefix argument. * Code style fix. * Fixed undefined variable error in test.
I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further. |
What does this PR do?
This PR adds a new argument to the
aws_key_pair
resource calledkey_name_prefix
. This behaves in the same way asname_prefix
seen on several other AWS resources.Any background context you want to provide?
It is worth noting that the current
key_name
argument will auto-generate an identifier if omitted. This was documented incorrectly which lead me down the path of implementing this (documentation now fixed in #9992).What are the relevant tickets?
I could not find an existing issue requesting this enhancement.
Checklist
Acceptance test coverage of new behaviour
Implemented - see
builtin/providers/aws/resource_aws_key_pair_test.go
Documentation updates
Implemented - see
website/source/docs/providers/aws/r/key_pair.html.markdown
Well-formed Code
make fmt
does not show any failures.