-
Notifications
You must be signed in to change notification settings - Fork 9.4k
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
Add aws codebuild source credential resource #7631
Add aws codebuild source credential resource #7631
Conversation
979bd0d
to
a6bf02c
Compare
Re-run acctest.
|
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.
Hi @kterada0509 👋 Thanks for submitting this. Just a few things and this should be good to go. Please reach out if you have any questions or do not have time to implement the feedback.
Create: resourceAwsCodeBuildSourceCredentialCreate, | ||
Read: resourceAwsCodeBuildSourceCredentialRead, | ||
Delete: resourceAwsCodeBuildSourceCredentialDelete, | ||
|
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.
Can you please implement resource import support? More information on the required steps can be found in the Contributing Guide. This resource testing will likely require ImportStateVerifyIgnore: []string{"token", "user_name"}
Thanks!
Importer: &schema.ResourceImporter{ | |
State: schema.ImportStatePassthrough, | |
}, | |
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.
fixed in c13110c
Token: aws.String(d.Get("token").(string)), | ||
} | ||
|
||
if attr, ok := d.GetOk("user_name"); ok && attr.(string) != "" && authType == codebuild.AuthTypeBasicAuth { |
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.
d.GetOk()
automatically checks for zero values so the empty string check is extraneous 👍
if attr, ok := d.GetOk("user_name"); ok && attr.(string) != "" && authType == codebuild.AuthTypeBasicAuth { | |
if attr, ok := d.GetOk("user_name"); ok && authType == codebuild.AuthTypeBasicAuth { |
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.
fixed in 64959a1
} | ||
|
||
d.SetId(aws.StringValue(resp.Arn)) | ||
d.Set("arn", resp.Arn) |
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.
Preferably the Create
function should return the Read
function and this d.Set()
should be handled in there. 👍
d.Set("arn", resp.Arn) |
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.
fixed in de6da2b
d.SetId(aws.StringValue(resp.Arn)) | ||
d.Set("arn", resp.Arn) | ||
|
||
return nil |
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.
Preferably the Create
function should return the Read
function.
return nil | |
return resourceAwsCodeBuildSourceCredentialRead(d, meta) |
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.
fixed in 05104d7
return nil | ||
} | ||
|
||
resourceNotFound := true |
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.
Typically we prefer to use relevant object type as the output variable when searching like this, which simplifies the logic and allows the code to read well on the left side:
var info *codebuild.SourceCredentialInfo
for _, sourceCredentialsInfo := range resp.SourceCredentialsInfos {
if d.Id() == aws.StringValue(sourceCredentialsInfo.Arn) {
info = sourceCredentialInfo
break
}
}
if info == nil {
log.Printf("[WARN] CodeBuild Source Credential (%s) not found, removing from state", d.Id())
d.SetId("")
return nil
}
d.Set("arn", info.Arn)
d.Set("auth_type", info.AuthType)
d.Set("server_type", info.ServerType)
return nil
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.
fixed in 588a1c3
} | ||
|
||
if _, err := conn.DeleteSourceCredentials(deleteOpts); err != nil { | ||
if !isAWSErr(err, codebuild.ErrCodeResourceNotFoundException, "") { |
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.
Typo 😎
if !isAWSErr(err, codebuild.ErrCodeResourceNotFoundException, "") { | |
if isAWSErr(err, codebuild.ErrCodeResourceNotFoundException, "") { |
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.
fixed in 588a1c3
Provides a CodeBuild Source Credential resource. | ||
--- | ||
|
||
# aws_codebuild_project |
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.
Copy pasta 🍝 and sitewide documentation update that occurred after this PR was submitted 👍
# aws_codebuild_project | |
# Resource: aws_codebuild_source_credential |
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.
fixed in a938f7a
a6bf02c
to
588a1c3
Compare
Re-run acctest.
@bflad ready for re-review. |
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.
Looks good, @kterada0509, thanks! 🚀 Will add to sidebar on merge and thinking it might be a good idea to mark the token attribute as Sensitive: true
as well. 😄
This has been released in version 2.22.0 of the Terraform AWS provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading. For further feature requests or bug reports with this functionality, please create a new GitHub issue following the template for triage. Thanks! |
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 feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. Thanks! |
Closes #7025
Closes #7435
Changes proposed in this pull request:
aws_codebuild_source_credential
resourceOutput from acceptance testing: