-
Notifications
You must be signed in to change notification settings - Fork 9.2k
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
Issue #4785: New Datasource aws_codecommit_repository #4934
Conversation
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 off to a great start per usual @saravanan30erd! A few things below and we should be able to get this in shortly!
FYI we are hanging out in https://gitter.im/hashicorp-terraform/gardening/ today for the gardening event if you'd like to chat in realtime
if isAWSErr(err, codecommit.ErrCodeRepositoryDoesNotExistException, "") { | ||
log.Printf("[WARN] CodeCommit Repository (%s) not found, removing from state", d.Id()) | ||
d.SetId("") | ||
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.
Currently, data sources should return a direct error instead of silently failing returning no results and likely returning a resource data.XXX.XXX not found for
error later on. 😄
return fmt.Errorf("Error reading CodeCommit Repository: %s", err.Error()) | ||
} | ||
} | ||
|
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.
To prevent potential panics, we should perform a nil
check for out.RepositoryMetadata
before referencing it below.
Config: testAccCheckAwsCodeCommitRepositoryDataSourceConfig(rName), | ||
Check: resource.ComposeTestCheckFunc( | ||
resource.TestCheckResourceAttr("data.aws_codecommit_repository.default", "repository_name", rName), | ||
resource.TestMatchResourceAttr("data.aws_codecommit_repository.default", "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.
Can we switch these to use resource.TestCheckResourceAttrPair()
where possible? This helps us ensure that the data source definitely matched the correct resource. Thanks! 👍
website/aws.erb
Outdated
@@ -103,6 +103,9 @@ | |||
<li<%= sidebar_current("docs-aws-datasource-cloudwatch-log-group") %>> | |||
<a href="/docs/providers/aws/d/cloudwatch_log_group.html">aws_cloudwatch_log_group</a> | |||
</li> | |||
<li<%= sidebar_current("docs-aws-datasource-codecommit-repository") %>> | |||
<a href="/docs/providers/aws/d/codecommit_repository.html">aws_cloudwatch_log_group</a> |
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.
Copypasta typo 🍝 here 😉
@bflad Done all the changes. $ make testacc TEST=./aws/ TESTARGS='-run=TestAccAWSCodeCommitRepositoryDataSource_basic' |
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 great, @saravanan30erd! Thanks! 🚀
1 test passed (all tests)
=== RUN TestAccAWSCodeCommitRepositoryDataSource_basic
--- PASS: TestAccAWSCodeCommitRepositoryDataSource_basic (8.56s)
Check: resource.ComposeTestCheckFunc( | ||
resource.TestCheckResourceAttrPair("data.aws_codecommit_repository.default", "repository_name", | ||
"aws_codecommit_repository.default", "repository_name"), | ||
resource.TestMatchResourceAttr("data.aws_codecommit_repository.default", "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.
Minor nitpick: I will update the rest of the checks to resource.TestCheckResourceAttrPair()
post merge.
This has been released in version 1.25.0 of the AWS provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading. |
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! |
Fixes #4785
Output from acceptance testing: