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

Add logs_config schema to aws_codebuild_project #7534

Merged
merged 2 commits into from
Jul 10, 2019

Conversation

srhaber
Copy link
Contributor

@srhaber srhaber commented Feb 13, 2019

Fixes #6312

Changes proposed in this pull request:

  • Adds logs_config parameter block to aws_codebuild_project.

Example:

resource "aws_codebuild_project" "test" {
...
  logs_config {
    cloudwatch_logs {
      status = "ENABLED|DISABLED"
      group_name = "..."
      stream_name = "..."
    }
    s3_logs {
      status = "ENABLED|DISABLED"
      location = "..." # ARN of an S3 bucket and the path prefix for S3 logs
    }
  }
...
}

Output from acceptance testing:

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

@ghost ghost added size/XL 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 Feb 13, 2019
@bflad bflad added the enhancement Requests to existing resources that expand the functionality or scope. label Mar 4, 2019
@srhaber
Copy link
Contributor Author

srhaber commented Apr 3, 2019

Hi, curious if there’s any reason this hasn’t been merged yet? It’s been 2 months, and there have been new releases to this provider during that span. I’ve waited patiently during that time and have received zero feedback.

Thanks,
-Shaun

@amellovmware
Copy link

Any thoughts when it will be merged?

@aeschright aeschright requested a review from a team June 25, 2019 22:12
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 @srhaber 👋 Thanks for this contribution and sorry for the delayed review. Please see the initial feedback and let us know if you have any questions or do not have time to implement the items.

Schema: map[string]*schema.Schema{
"status": {
Type: schema.TypeString,
Required: true,
Copy link
Contributor

Choose a reason for hiding this comment

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

Typically for these types of enabled/disabled flag arguments, we would prefer to make it optional and instead default to match the API default. Since the attribute is using DiffSuppressFunc to ignore enabled configurations, it would be simpler to declare this attribute as:

"status": {
	Type:     schema.TypeString,
	Optional: true,
	Default:  codebuild.LogsConfigStatusTypeEnabled,
	ValidateFunc: validation.StringInSlice([]string{
		codebuild.LogsConfigStatusTypeDisabled,
		codebuild.LogsConfigStatusTypeEnabled,
	}, false),
},

},
},
},
DiffSuppressFunc: func(k, old, new string, d *schema.ResourceData) bool {
Copy link
Contributor

Choose a reason for hiding this comment

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

This can be simplified with our helper function: DiffSuppressFunc: suppressMissingOptionalConfigurationBlock,

aws/resource_aws_codebuild_project.go Show resolved Hide resolved
Optional: true,
ValidateFunc: validateAwsCodeBuildProjectS3LogsLocation,
DiffSuppressFunc: func(k, old, new string, d *schema.ResourceData) bool {
return strings.TrimPrefix(new, "arn:aws:s3:::") == old
Copy link
Contributor

Choose a reason for hiding this comment

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

This particular difference suppression will only work in AWS Commercial (aws partition) -- AWS supports many other partitions including AWS China (aws-cn), AWS GovCloud US (aws-us-gov), and some other non-public ones with their own ARN formats.

In general, we should avoid implementing our own logic on top of the API and instead validate the input to match the canonical form that is returned from the API. If the API always returns the value in the non-ARN format, we should validate that an ARN is not given and vice-versa. 👍

},
},
},
DiffSuppressFunc: func(k, old, new string, d *schema.ResourceData) bool {
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here regarding suppressMissingOptionalConfigurationBlock

@@ -781,6 +949,10 @@ func resourceAwsCodeBuildProjectRead(d *schema.ResourceData, meta interface{}) e
return err
}

if err := d.Set("logs_config", flattenAwsCodeBuildLogsConfig(project.LogsConfig)); err != nil {
return err
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: The code around this isn't doing it (😖), but we should return context for operators and maintainers for errors like these, e.g.

Suggested change
return err
return fmt.Errorf("error setting logs_config: %s", err)


func flattenAwsCodeBuildCloudWatchLogs(cloudWatchLogsConfig *codebuild.CloudWatchLogsConfig) []interface{} {
if cloudWatchLogsConfig == nil {
return []interface{}{}
Copy link
Contributor

Choose a reason for hiding this comment

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

We will likely need to return the default status value here if the API response is completely empty, so Terraform does not report an unexpected plan difference:

Suggested change
return []interface{}{}
return []interface{}{
"status": codebuild.LogsConfigStatusTypeEnabled,
}


func flattenAwsCodeBuildS3Logs(s3LogsConfig *codebuild.S3LogsConfig) []interface{} {
if s3LogsConfig == nil {
return []interface{}{}
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here: We will likely need to return the default status value here if the API response is completely empty, so Terraform does not report an unexpected plan difference:

Suggested change
return []interface{}{}
return []interface{}{
"status": codebuild.LogsConfigStatusTypeDisabled,
}

aws/resource_aws_codebuild_project_test.go Show resolved Hide resolved

s3_logs {
status = "ENABLED"
location = "${aws_s3_bucket.example.arn}/build-log"
Copy link
Contributor

Choose a reason for hiding this comment

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

Given the above changes for bucket name instead of ARN, this would be replaced with:

Suggested change
location = "${aws_s3_bucket.example.arn}/build-log"
location = "${aws_s3_bucket.example.bucket}/build-log"

@bflad bflad added the waiting-response Maintainers are waiting on response from community or contributor. label Jul 8, 2019
@bflad bflad self-assigned this Jul 8, 2019
@srhaber
Copy link
Contributor Author

srhaber commented Jul 9, 2019

Thanks for the feedback @bflad! I'll have some updates pushed up shortly.

@ghost ghost removed the waiting-response Maintainers are waiting on response from community or contributor. label Jul 9, 2019
@bflad bflad added the waiting-response Maintainers are waiting on response from community or contributor. label Jul 9, 2019
@srhaber srhaber force-pushed the aws_codebuild_project_logs_config branch from 2836980 to 52e2621 Compare July 9, 2019 18:47
@srhaber
Copy link
Contributor Author

srhaber commented Jul 9, 2019

I implemented the suggested changes above. It required a rebase from master and corresponding force-push since there was a bit of code drift over the past few months.

I also added the new encryption_disabled attribute for the s3_logs config.

Output from updated acceptance tests:

make testacc TESTARGS='-run=TestAccAWSCodeBuildProject_\(basic\|LogsConfig\)'                                                                                                                                                  Tue Jul 09 11:45:18 [10682]
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./... -v -parallel 20 -run=TestAccAWSCodeBuildProject_\(basic\|LogsConfig\) -timeout 120m
?   	github.com/srhaber/terraform-provider-aws	[no test files]
=== RUN   TestAccAWSCodeBuildProject_basic
=== PAUSE TestAccAWSCodeBuildProject_basic
=== RUN   TestAccAWSCodeBuildProject_LogsConfig_CloudWatchLogs
=== PAUSE TestAccAWSCodeBuildProject_LogsConfig_CloudWatchLogs
=== RUN   TestAccAWSCodeBuildProject_LogsConfig_S3Logs
=== PAUSE TestAccAWSCodeBuildProject_LogsConfig_S3Logs
=== CONT  TestAccAWSCodeBuildProject_basic
=== CONT  TestAccAWSCodeBuildProject_LogsConfig_S3Logs
=== CONT  TestAccAWSCodeBuildProject_LogsConfig_CloudWatchLogs
--- PASS: TestAccAWSCodeBuildProject_basic (32.91s)
--- PASS: TestAccAWSCodeBuildProject_LogsConfig_CloudWatchLogs (48.26s)
--- PASS: TestAccAWSCodeBuildProject_LogsConfig_S3Logs (54.22s)
PASS
ok  	github.com/srhaber/terraform-provider-aws/aws	55.652s

@ghost ghost removed the waiting-response Maintainers are waiting on response from community or contributor. label Jul 9, 2019
@bflad bflad added this to the v2.19.0 milestone Jul 10, 2019
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.

Thanks so much for the updates, @srhaber! Looks great. 🚀

--- PASS: TestAccAWSCodeBuildProject_Source_Auth (25.63s)
--- PASS: TestAccAWSCodeBuildProject_Source_Type_Bitbucket (25.63s)
--- PASS: TestAccAWSCodeBuildProject_basic (35.39s)
--- PASS: TestAccAWSCodeBuildProject_BuildTimeout (36.13s)
--- PASS: TestAccAWSCodeBuildProject_Source_GitCloneDepth (36.28s)
--- PASS: TestAccAWSCodeBuildProject_Source_Type_CodeCommit (37.27s)
--- PASS: TestAccAWSCodeBuildProject_BadgeEnabled (37.62s)
--- PASS: TestAccAWSCodeBuildProject_Source_ReportBuildStatus_GitHub (38.04s)
--- PASS: TestAccAWSCodeBuildProject_importBasic (38.59s)
--- PASS: TestAccAWSCodeBuildProject_Environment_Certificate (40.74s)
--- PASS: TestAccAWSCodeBuildProject_Description (47.64s)
--- PASS: TestAccAWSCodeBuildProject_LogsConfig_S3Logs (48.26s)
--- PASS: TestAWSCodeBuildProject_nameValidation (0.00s)
--- PASS: TestAccAWSCodeBuildProject_Source_ReportBuildStatus_Bitbucket (48.52s)
--- PASS: TestAccAWSCodeBuildProject_Environment_EnvironmentVariable_Type (48.79s)
--- PASS: TestAccAWSCodeBuildProject_Source_ReportBuildStatus_GitHubEnterprise (48.93s)
--- PASS: TestAccAWSCodeBuildProject_Source_InsecureSSL (48.97s)
--- PASS: TestAccAWSCodeBuildProject_Source_Type_NoSourceInvalid (15.11s)
--- PASS: TestAccAWSCodeBuildProject_LogsConfig_CloudWatchLogs (55.25s)
--- PASS: TestAccAWSCodeBuildProject_Source_Type_CodePipeline (31.04s)
--- PASS: TestAccAWSCodeBuildProject_EncryptionKey (57.18s)
--- PASS: TestAccAWSCodeBuildProject_Source_Type_GitHubEnterprise (31.32s)
--- PASS: TestAccAWSCodeBuildProject_Source_Type_NoSource (21.72s)
--- PASS: TestAccAWSCodeBuildProject_Source_Type_S3 (22.71s)
--- PASS: TestAccAWSCodeBuildProject_Environment_EnvironmentVariable (58.60s)
--- PASS: TestAccAWSCodeBuildProject_Artifacts_EncryptionDisabled (22.87s)
--- PASS: TestAccAWSCodeBuildProject_SecondaryArtifacts (22.35s)
--- PASS: TestAccAWSCodeBuildProject_WindowsContainer (29.36s)
--- PASS: TestAccAWSCodeBuildProject_Tags (37.58s)
--- PASS: TestAccAWSCodeBuildProject_SecondarySources_CodeCommit (27.80s)
--- PASS: TestAccAWSCodeBuildProject_Cache (76.38s)
--- PASS: TestAccAWSCodeBuildProject_Environment_RegistryCredential (27.86s)

@@ -826,31 +980,35 @@ func resourceAwsCodeBuildProjectRead(d *schema.ResourceData, meta interface{}) e
project := resp.Projects[0]

if err := d.Set("artifacts", flattenAwsCodeBuildProjectArtifacts(project.Artifacts)); err != nil {
return err
return fmt.Errorf("error setting artifacts: %s", err)
Copy link
Contributor

Choose a reason for hiding this comment

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

😍 Thank you!

@bflad bflad merged commit 0f1eafd into hashicorp:master Jul 10, 2019
bflad added a commit that referenced this pull request Jul 10, 2019
@bflad
Copy link
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
Copy link

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 ghost locked and limited conversation to collaborators Nov 2, 2019
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. enhancement Requests to existing resources that expand the functionality or scope. service/codebuild Issues and PRs that pertain to the codebuild service. size/XL 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.

In Terraform 0.11.8, aws_codebuild_project resource does not expose logsConfig from AWS API
3 participants