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

Support for setting of certificate in aws_codebuild_project #6087

Merged
merged 2 commits into from
Oct 9, 2018

Conversation

tkbky
Copy link
Contributor

@tkbky tkbky commented Oct 8, 2018

Fixes #4742

This change allows the certificate to be set for an aws_codebuild_project.

Output from acceptance testing:

$ make testacc TESTARGS='-run= TestAccAWSCodeBuildProject_Environment_Certificate'

TF_ACC=1 go test ./... -v -run=TestAccAWSCodeBuildProject_Environment_Certificate -timeout 120m
?   	github.com/terraform-providers/terraform-provider-aws	[no test files]
=== RUN   TestAccAWSCodeBuildProject_Environment_Certificate
--- PASS: TestAccAWSCodeBuildProject_Environment_Certificate (79.98s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	80.019s
...

@ghost ghost added size/M Managed by automation to categorize the size of a PR. documentation Introduces or discusses updates to documentation. service/codebuild Issues and PRs that pertain to the codebuild service. tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure. labels Oct 8, 2018
Copy link
Contributor

@bflad bflad left a comment

Choose a reason for hiding this comment

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

Hi @tkbky! Thanks for this pull request as well! Left some initial code-level feedback here and it looks like this might require some additional acceptance testing setup due to your comment in #4742. Can you let us know if this fails acceptance testing without a proper setup? Cheers!

@@ -984,6 +992,7 @@ func flattenAwsCodeBuildProjectEnvironment(environment *codebuild.ProjectEnviron
envConfig["type"] = *environment.Type
envConfig["compute_type"] = *environment.ComputeType
envConfig["image"] = *environment.Image
envConfig["certificate"] = *environment.Certificate
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you please switch this to the AWS Go SDK helper function to prevent panics? e.g.

envConfig["certificate"] = aws.StringValue(environment.Certificate)

We should probably fix the others right here too but not necessary in this pull request.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, will fix this up.

compute_type = "BUILD_GENERAL1_SMALL"
image = "2"
type = "LINUX_CONTAINER"
certificate = "arn:aws:s3:::secret_bucket/cert.pem"
Copy link
Contributor

Choose a reason for hiding this comment

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

Two things here:

  • Should we hook this up to a real S3 bucket and object? e.g. aws_s3_bucket and aws_s3_bucket_object resource If CodeBuild does not require a valid path to a valid object, I think we can just stick using the aws_s3_bucket resource
  • Nit: Looks like the formatting might be a little off here (tabs vs spaces)

@bflad bflad added the waiting-response Maintainers are waiting on response from community or contributor. label Oct 8, 2018
@tkbky tkbky force-pushed the aws-code-build-certificate branch from 0c47fb6 to c5c262b Compare October 9, 2018 15:01
@tkbky
Copy link
Contributor Author

tkbky commented Oct 9, 2018

@bflad I've managed to set up the acceptance test for the certificate test case. Previously, it fails other acceptance tests due to the certificate was set to empty string which raises the invalid extension error.

@tkbky tkbky changed the title WIP Support for setting of certificate in aws_codebuild_project Support for setting of certificate in aws_codebuild_project Oct 9, 2018
@bflad bflad removed the waiting-response Maintainers are waiting on response from community or contributor. label Oct 9, 2018
Copy link
Contributor

@bflad bflad left a comment

Choose a reason for hiding this comment

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

Great work here, @tkbky! This was really close and a little more complex than necessary due to the TypeSet handling required, but should be good to go after 1a4c37f 🚀

--- PASS: TestAccAWSCodeBuildProject_importBasic (16.69s)
--- PASS: TestAccAWSCodeBuildProject_BadgeEnabled (16.76s)
--- PASS: TestAccAWSCodeBuildProject_Source_Type_CodeCommit (16.71s)
--- PASS: TestAccAWSCodeBuildProject_Source_Type_Bitbucket (17.21s)
--- PASS: TestAccAWSCodeBuildProject_Source_Type_S3 (17.36s)
--- PASS: TestAccAWSCodeBuildProject_Environment_Certificate (17.53s)
--- PASS: TestAccAWSCodeBuildProject_Source_Type_GitHubEnterprise (17.61s)
--- PASS: TestAccAWSCodeBuildProject_Environment_EnvironmentVariable_Type (21.80s)
--- PASS: TestAccAWSCodeBuildProject_Source_Type_CodePipeline (25.11s)
--- PASS: TestAccAWSCodeBuildProject_basic (25.63s)
--- PASS: TestAccAWSCodeBuildProject_Source_Auth (25.68s)
--- PASS: TestAccAWSCodeBuildProject_Source_GitCloneDepth (29.00s)
--- PASS: TestAccAWSCodeBuildProject_Tags (29.46s)
--- PASS: TestAccAWSCodeBuildProject_Description (29.63s)
--- PASS: TestAccAWSCodeBuildProject_Source_ReportBuildStatus (29.56s)
--- PASS: TestAccAWSCodeBuildProject_BuildTimeout (29.72s)
--- PASS: TestAccAWSCodeBuildProject_Source_InsecureSSL (29.74s)
--- PASS: TestAccAWSCodeBuildProject_WindowsContainer (15.57s)
--- PASS: TestAccAWSCodeBuildProject_Cache (33.83s)
--- PASS: TestAccAWSCodeBuildProject_VpcConfig (35.29s)
--- PASS: TestAccAWSCodeBuildProject_Artifacts_EncryptionDisabled (23.55s)
--- PASS: TestAccAWSCodeBuildProject_SecondaryArtifacts (23.88s)
--- PASS: TestAccAWSCodeBuildProject_SecondarySources_CodeCommit (23.77s)
--- PASS: TestAccAWSCodeBuildProject_EncryptionKey (42.47s)

@@ -177,6 +177,10 @@ func resourceAwsCodeBuildProject() *schema.Resource {
Optional: true,
Default: false,
},
"certificate": {
Copy link
Contributor

Choose a reason for hiding this comment

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

Since the CodeBuild API seems to validate against .pem and .zip endings, we can implement plan-time validation for that via:

ValidateFunc: validation.StringMatch(regexp.MustCompile(`\.(pem|zip)$`), "must end in .pem or .zip"),

Also, since this attribute lives inside a Type: schema.TypeSet attribute, there is a Set function which is used to detect changes. resourceAwsCodeBuildProjectEnvironmentHash in this case.

We can add the following there:

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

At some point we'll likely be converting many of these TypeSet attributes, due to their unnecessary complexity.

Config: testAccAWSCodeBuildProjectConfig_Environment_Certificate(rName, bName, oName),
Check: resource.ComposeTestCheckFunc(
testAccCheckAWSCodeBuildProjectExists(resourceName, &project),
resource.TestCheckResourceAttr(resourceName, "environment.1974383098.certificate", fmt.Sprintf("%s/%s", bName, oName)),
Copy link
Contributor

Choose a reason for hiding this comment

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

Since the TypeSet value wasn't dependent on the certificate value previously, the 1974383098 value was static. Now that resourceAwsCodeBuildProjectEnvironmentHash has been updated and since we randomly generate the S3 bucket name, the hash will never be consistent. Rather than trying to recompute the hash value for this test, we can instead just verify the API is set correctly via:

testAccCheckAWSCodeBuildProjectCertificate(&project, fmt.Sprintf("%s/%s", bName, oName)),

and its definition:

func testAccCheckAWSCodeBuildProjectCertificate(project *codebuild.Project, expectedCertificate string) resource.TestCheckFunc {
	return func(s *terraform.State) error {
		if aws.StringValue(project.Environment.Certificate) != expectedCertificate {
			return fmt.Errorf("CodeBuild Project certificate (%s) did not match: %s", aws.StringValue(project.Environment.Certificate), expectedCertificate)
		}
		return nil
	}
}

@@ -619,6 +623,10 @@ func expandProjectEnvironment(d *schema.ResourceData) *codebuild.ProjectEnvironm
projectEnv.Type = aws.String(v.(string))
}

if v := envConfig["certificate"]; v != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

To prevent the Terraform resource from always triggering this error when not set in the Terrraform configuration:

--- FAIL: TestAccAWSCodeBuildProject_basic (6.15s)
    testing.go:527: Step 0 error: Error applying: 1 error occurred:
        	* aws_codebuild_project.test: 1 error occurred:
        	* aws_codebuild_project.test: Error creating CodeBuild project: InvalidInputException: Invalid extension: certificate must be either .pem or .zip
        	status code: 400, request id: bc162507-cbe4-11e8-aeb7-db9c8472a837

We can use this instead:

if v, ok := envConfig["certificate"]; ok && v.(string) != "" {
	projectEnv.Certificate = aws.String(v.(string))
}

@bflad bflad added this to the v1.40.0 milestone Oct 9, 2018
@bflad bflad merged commit ca8a2c2 into hashicorp:master Oct 9, 2018
bflad added a commit that referenced this pull request Oct 9, 2018
@tkbky
Copy link
Contributor Author

tkbky commented Oct 9, 2018

@bflad Thank you for your time go through this PR and all your comments are valuable to my learning, really appreciate that. 😃

@bflad
Copy link
Contributor

bflad commented Oct 10, 2018

This has been released in version 1.40.0 of the AWS provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

@ghost
Copy link

ghost commented Apr 2, 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 feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. Thanks!

@ghost ghost locked and limited conversation to collaborators Apr 2, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
documentation Introduces or discusses updates to documentation. service/codebuild Issues and PRs that pertain to the codebuild service. size/M Managed by automation to categorize the size of a PR. tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Can not set certificate with aws_codebuild_project?
2 participants