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.

Sign up for GitHub

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

Jump to bottom

resource/aws_codebuild_project: Add registry_credential argument for environment #9168

Merged

bflad merged 4 commits into hashicorp:master from iliazlobin:master

Jul 8, 2019

iliazlobin commented Jun 27, 2019

Community Note

Please vote on this pull request by adding a 👍 reaction to the original pull request comment to help the community and maintainers prioritize this request
Please do not leave "+1" comments, they generate extra noise for pull request followers and do not help prioritize the request

Closes #9147 #8028

Release note for CHANGELOG:

* resource/aws_codebuild_project: Add `registry_credential` attribute ([#9147](https://github.com/terraform-providers/terraform-provider-aws/issues/9147))

Output from acceptance testing:

$ make testacc TESTARGS='-run=TestAccAWSCodeBuildProject_Environment_RegistryCredential'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./... -v -parallel 20 -run=TestAccAWSCodeBuildProject_Environment_RegistryCredential -timeout 120m
?       github.com/terraform-providers/terraform-provider-aws   [no test files]
=== RUN   TestAccAWSCodeBuildProject_Environment_RegistryCredential
=== PAUSE TestAccAWSCodeBuildProject_Environment_RegistryCredential
=== CONT  TestAccAWSCodeBuildProject_Environment_RegistryCredential
--- PASS: TestAccAWSCodeBuildProject_Environment_RegistryCredential (48.38s)
PASS
ok      github.com/terraform-providers/terraform-provider-aws/aws       54.721s

iliazlobin-epam added 3 commits

June 27, 2019 18:22


          add registry credential configuration option for codebuild_project en…

77ffe18

…vironment


          update codebuild_project docs

c3fc893


          fix acceptance test

738fd2f

iliazlobin requested a review from a team

June 27, 2019 16:25

ghost added size/M documentation service/codebuild tests labels

bflad suggested changes

View reviewed changes

Contributor

bflad left a comment

Hi @iliazlobin 👋 Thanks so much for this contribution, its off to a great start. A few minor things below and we should be able to get this in. Please reach out if you have any questions or do not have time to implement the feedback items. 👍

aws/resource_aws_codebuild_project.go Outdated

@@ @@ -197,6 +197,26 @@ func resourceAwsCodeBuildProject() *schema.Resource { @@
               							Optional:     true,
               							ValidateFunc: validation.StringMatch(regexp.MustCompile(`\.(pem|zip)$`), "must end in .pem or .zip"),
               						},
+              						"registry_credential": {
+              							Type:     schema.TypeSet,

Contributor

bflad Jun 27, 2019

When using MaxItems: 1 for configuration block attributes, we prefer to simply the attribute type to TypeList instead. 👍

Suggested change

      
            							Type:     schema.TypeSet,
          
            							Type:     schema.TypeList,

aws/resource_aws_codebuild_project.go Outdated

               		projectEnv.ImagePullCredentialsType = aws.String(v.(string))
               	}
+              	if v := envConfig["registry_credential"]; v != nil {

Contributor

bflad Jun 27, 2019

It looks like the code around this might not be performing the safer check, but could you please update this to something like the following to perform the existence checking? Thanks!

Suggested change

      
            	if v := envConfig["registry_credential"]; v != nil {
          
            	if v, ok := envConfig["registry_credential"]; ok && len(v.([]interface{})) > 0 {

aws/resource_aws_codebuild_project.go Outdated

               		projectEnv.ImagePullCredentialsType = aws.String(v.(string))
               	}
+              	if v := envConfig["registry_credential"]; v != nil {
+              		config := v.(*schema.Set).List()[0].(map[string]interface{})

Contributor

bflad Jun 27, 2019

With the switch to TypeList and the length check added above for safety, this simplifies to:

Suggested change

      
            		config := v.(*schema.Set).List()[0].(map[string]interface{})
          
            		config := v.([]interface{}).[0].(map[string]interface{})

aws/resource_aws_codebuild_project.go Outdated


		projectRegistryCredential := &codebuild.RegistryCredential{}

		if v := config["credential"].(string); v != "" {

Contributor

bflad Jun 27, 2019

For additional safety we should use existence checking here as well. 👍

Suggested change

      
            		if v := config["credential"].(string); v != "" {
          
            		if v, ok := config["credential"]; ok && v.(string) != "" {

aws/resource_aws_codebuild_project.go Outdated

+              		projectRegistryCredential := &codebuild.RegistryCredential{}
+              		if v := config["credential"].(string); v != "" {
+              			projectRegistryCredential.Credential = &v

Contributor

bflad Jun 27, 2019

Generally when setting AWS Go SDK parameters, we prefer (by convention) to use the provided conversion functions (e.g. aws.String()) instead of raw Go pointer referencing

Suggested change

      
            			projectRegistryCredential.Credential = &v
          
            			projectRegistryCredential.Credential = aws.String(v.(string))

aws/resource_aws_codebuild_project.go Outdated

+              			projectRegistryCredential.Credential = &v
+              		}
+              		if v := config["credential_provider"].(string); v != "" {

Contributor

bflad Jun 27, 2019

Similar change here:

Suggested change

      
            		if v := config["credential_provider"].(string); v != "" {
          
            		if v, ok := config["credential_provider"]; ok && v.(string) != "" {

aws/resource_aws_codebuild_project.go Outdated

+              		}
+              		if v := config["credential_provider"].(string); v != "" {
+              			projectRegistryCredential.CredentialProvider = &v

Contributor

bflad Jun 27, 2019

Similar change here:

Suggested change

      
            			projectRegistryCredential.CredentialProvider = &v
          
            			projectRegistryCredential.CredentialProvider = aws.String(v.(string))

aws/resource_aws_codebuild_project_test.go Show resolved Hide resolved

aws/resource_aws_codebuild_project_test.go Outdated

+                environment {
+                  compute_type                = "BUILD_GENERAL1_SMALL"
+                  image                       = "2"
+              		type                        = "LINUX_CONTAINER"

Contributor

bflad Jun 27, 2019

Can you please fix the formatting here? Looks like spaces versus tabs. 😄

Suggested change

      
            		type                        = "LINUX_CONTAINER"
          
              type                        = "LINUX_CONTAINER"

aws/resource_aws_codebuild_project_test.go Outdated

+                  compute_type                = "BUILD_GENERAL1_SMALL"
+                  image                       = "2"
+              		type                        = "LINUX_CONTAINER"
+              		image_pull_credentials_type = "SERVICE_ROLE"

Contributor

bflad Jun 27, 2019

Suggested change

      
            		image_pull_credentials_type = "SERVICE_ROLE"
          
              image_pull_credentials_type = "SERVICE_ROLE"

bflad self-assigned this

bflad added enhancement waiting-response labels


          code review issues

e099db0

Author

iliazlobin commented Jun 28, 2019

Hi @bflad! Thank you for reviewing my code and putting clear notes of what needs to be tuned.
It was a pleasure to bring a useful and actual feature to the project.
I'm kind of new in Go, but based on my experience in other languages, I was able to work it out.
I should admit it wasn't difficult to figure out what part of the code needs to change to have the required functionality. The code is well structured and understandable. Huge respect for that!

bflad self-requested a review

July 1, 2019 12:57

bflad removed the waiting-response label

bflad added this to the v2.19.0 milestone

bflad approved these changes

View reviewed changes

Contributor

bflad left a comment

Thanks so much for the updates here, @iliazlobin! Your changes looked good! 🚀

Output from acceptance testing:

--- PASS: TestAccAWSCodeBuildProject_Source_Type_Bitbucket (24.02s)
--- PASS: TestAccAWSCodeBuildProject_basic (24.13s)
--- PASS: TestAccAWSCodeBuildProject_Source_Auth (24.35s)
--- PASS: TestAccAWSCodeBuildProject_SecondarySources_CodeCommit (24.40s)
--- PASS: TestAccAWSCodeBuildProject_importBasic (25.59s)
--- PASS: TestAccAWSCodeBuildProject_Source_ReportBuildStatus_Bitbucket (32.31s)
--- PASS: TestAccAWSCodeBuildProject_Source_ReportBuildStatus_GitHubEnterprise (32.63s)
--- PASS: TestAccAWSCodeBuildProject_Source_GitCloneDepth (32.90s)
--- PASS: TestAccAWSCodeBuildProject_BuildTimeout (33.12s)
--- PASS: TestAccAWSCodeBuildProject_Environment_EnvironmentVariable_Type (33.13s)
--- PASS: TestAccAWSCodeBuildProject_Description (33.84s)
--- PASS: TestAccAWSCodeBuildProject_BadgeEnabled (33.93s)
--- PASS: TestAccAWSCodeBuildProject_SecondaryArtifacts (35.97s)
--- PASS: TestAccAWSCodeBuildProject_Artifacts_EncryptionDisabled (36.12s)
--- PASS: TestAccAWSCodeBuildProject_Environment_Certificate (39.08s)
--- PASS: TestAccAWSCodeBuildProject_Source_Type_NoSourceInvalid (7.32s)
--- PASS: TestAccAWSCodeBuildProject_Source_InsecureSSL (40.11s)
--- PASS: TestAccAWSCodeBuildProject_Environment_RegistryCredential (41.34s)
--- PASS: TestAccAWSCodeBuildProject_Environment_EnvironmentVariable (42.96s)
--- PASS: TestAccAWSCodeBuildProject_WindowsContainer (20.95s)
--- PASS: TestAccAWSCodeBuildProject_Source_Type_GitHubEnterprise (20.79s)
--- PASS: TestAccAWSCodeBuildProject_Source_Type_CodePipeline (21.18s)
--- PASS: TestAccAWSCodeBuildProject_EncryptionKey (52.34s)
--- PASS: TestAccAWSCodeBuildProject_Source_Type_NoSource (20.67s)
--- PASS: TestAccAWSCodeBuildProject_Source_Type_S3 (32.59s)
--- PASS: TestAccAWSCodeBuildProject_Source_Type_CodeCommit (32.69s)
--- PASS: TestAccAWSCodeBuildProject_Tags (28.28s)
--- PASS: TestAccAWSCodeBuildProject_Source_ReportBuildStatus_GitHub (29.05s)
--- PASS: TestAccAWSCodeBuildProject_Cache (62.92s)

There is one very subtle detail that even our acceptance testing cannot properly catch in this scenario and its only related to some technical debt we have planned to clean up in #6427 I'm leaving full details for this below, but this is not something we would typically expect outside contributors to pick up on at all.

For updates to properly occur with TypeSet attributes that have a custom Set function declared, new attributes must be added to the hashing function. The Terraform Provider SDK uses the hash value to determine if the parent attribute needs to be updated, otherwise configuration updates will not result in API updates and Terraform will show no updates.

In general, proper TypeSet attribute testing is very problematic in the current Terraform Provider SDK framework. Usually ImportStateVerify can uncover issues with the state values, but unfortunately it cannot easily pick up on these particular hashing issues since we cannot:

Easily use resource.TestCheckResourceAttr() (or in this particular case resource.TestCheckResourceAttrPair()) since the environment hash value in the flatmap would be dynamic due to the dynamic value of the underlying credential ARN, we'd have to rely on checking the API response value in &project from testAccCheckAWSCodeBuildProjectExists
Determine the missing change with ImportStateVerify testing

To fix this particular issue, it was just a matter of adding this to resourceAwsCodeBuildProjectEnvironmentHash in a followup commit (5d5dcbb):

	if v, ok := m["registry_credential"]; ok && len(v.([]interface{})) > 0 && v.([]interface{})[0] != nil {
		m := v.([]interface{})[0].(map[string]interface{})

		if v, ok := m["credential"]; ok && v.(string) != "" {
			buf.WriteString(fmt.Sprintf("%s-", v.(string)))
		}

		if v, ok := m["credential_provider"]; ok && v.(string) != "" {
			buf.WriteString(fmt.Sprintf("%s-", v.(string)))
		}
	}

In the future we'll be replacing these TypeSet and MaxItems: 1 attributes with TypeList, so this particular issue cannot occur.

bflad merged commit e099db0 into hashicorp:master

bflad added a commit that referenced this pull request


          Update CHANGELOG for #9168

b6065fc

Contributor

bflad commented Jul 11, 2019

This has been released in version 2.19.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!

ghost commented Nov 2, 2019

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!

ghost locked and limited conversation to collaborators

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

documentation enhancement service/codebuild size/M tests