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

Added KMS alias name_prefix #5594

Closed
wants to merge 4 commits into from
Closed

Added KMS alias name_prefix #5594

wants to merge 4 commits into from

Conversation

iceycake
Copy link
Contributor

Instead of a static KMS alias name, it allows either name_prefix or generate unique name.

@iceycake iceycake changed the title ISSUE-5592: Added testing for the kms name_prefix Added KMS alias name_prefix Mar 11, 2016
} else {
name = resource.UniqueId()
}

Copy link
Contributor

Choose a reason for hiding this comment

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

since we're now modifying what we get from the config regarding the name, we need to make sure we use the d.Id() in resourceAwsKmsAliasRead, because d.Get("name") may now return ""

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do I need to make any change here? The name was in the old implementation so I just keep it. I saw similar implementation with security_group. Please let me know if I need to fix this for the merge or it's something we can deal with with another pull request.

@catsby
Copy link
Contributor

catsby commented Mar 11, 2016

Looks good! I left a note about the usage of name vs. Id, and I believe we'll need an update to the documentation as well before we can merge.

@catsby catsby added enhancement waiting-response An issue/pull request is waiting for a response from the community provider/aws labels Mar 11, 2016
@catsby
Copy link
Contributor

catsby commented Mar 11, 2016

Also, I ran the TestAccAWSKmsAlias_no_name test and got a failure, can you take a look?

--- FAIL: TestAccAWSKmsAlias_no_name (143.46s)
    testing.go:148: Step 0 error: Error applying: 1 error(s) occurred:

        * aws_kms_alias.nothing: ValidationException: Alias must start with the prefix "alias/". Please see http://docs.aws.amazon.com/kms/latest/developerguide/programming-aliases.html
            status code: 400, request id: 5ccc078e-e7da-11

@stack72 stack72 removed the waiting-response An issue/pull request is waiting for a response from the community label Mar 14, 2016
@iceycake
Copy link
Contributor Author

Is this ready to be accepted?

@stack72
Copy link
Contributor

stack72 commented Mar 21, 2016

Hi @iceycake

thanks so much for fixing up the rest of the rest as per @catsby's notes

The tests now pass as follows so this looks good to merge

make testacc TEST=./builtin/providers/azurerm TESTARGS='-run=TestAccAWSKmsAlias' 2>~/tf.log
==> Checking that code complies with gofmt requirements...
/Users/stacko/Code/go/bin/stringer
go generate $(go list ./... | grep -v /vendor/)
TF_ACC=1 go test ./builtin/providers/azurerm -v -run=TestAccAWSKmsAlias -timeout 120m
PASS
ok      github.com/hashicorp/terraform/builtin/providers/azurerm    0.011s

@stack72
Copy link
Contributor

stack72 commented Mar 21, 2016

Merged in 9cfcfed

@stack72 stack72 closed this Mar 21, 2016
@ghost
Copy link

ghost commented Apr 27, 2020

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.

@ghost ghost locked and limited conversation to collaborators Apr 27, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants