Add cache option to AWS CodeBuild projects #2860

kaofelix · 2018-01-04T10:04:45Z

This is for implementing support for the cache feature I requested in #2842.

nathanielks · 2018-01-06T02:19:07Z

@kaofelix Thanks for putting this together! I was going to add this myself. I'm not a maintainer, but on the last PR I submitted, they asked that validators be put into https://github.com/terraform-providers/terraform-provider-aws/blob/master/aws/validators.go. I imagine they'll ask the same thing here!

kaofelix · 2018-01-06T10:21:15Z

Hey @nathanielks, thanks for the heads up! I was not aware of the validators.go file. I ended up putting it in there since that's were I found the other validators for CodeBuild projects. Do you think it would make sense to also move the other validators to this file, or would a refactoring like this normally be out of scope for a PR about a more specific feature? It does feel kind of odd to just move my own new validator to that file and leave the others still in resource_aws_codebuild_project.go, but I also don't want to go overboard on the changes, what do you think?

nathanielks · 2018-01-07T22:13:48Z

@kaofelix I'd say that's out of scope for this PR. They just had me add my individual validator to the file, as well as move my test to validators_test.go. I'm sure they wouldn't mind a separate PR moving them!

nathanielks · 2018-01-07T22:14:40Z

For reference: #2090 (comment)

kaofelix · 2018-01-08T09:15:56Z

Cool, @nathanielks! I moved it to the validators file, thanks again for the heads up :)

radeksimko

Hi @kaofelix
thanks for raising the PR.

I left you some comments there.

Just FYI for future PRs - it may be helpful/easier for you (and others) to always raise a PR from a branch, rather than master. It is easier to collaborate (if necessary), you can raise multiple PRs to the same repo without conflicts and finally you don't need to force-push to master when rebasing and/or squashing commits.

radeksimko · 2018-01-08T10:41:35Z

aws/resource_aws_codebuild_project.go

@@ -59,6 +59,25 @@ func resourceAwsCodeBuildProject() *schema.Resource {
 				},
 				Set: resourceAwsCodeBuildProjectArtifactsHash,
 			},
+			"cache": {
+				Type:     schema.TypeSet,


Since there won't ever be more than 1 cache stanza we should make it TypeList instead as there's no point in computing the hash for a set and dealing with ordering (as there's nothing to order). Also it will be ~~easier~~ possible to reference nested fields like ${aws_codebuild_project.cache.0.type} etc.

Ok, I've changed it into a List now, seems to work fine. Thanks for the tip!

radeksimko · 2018-01-08T10:42:00Z

aws/resource_aws_codebuild_project.go

+						},
+					},
+				},
+				Set: resourceAwsCodeBuildProjectCacheHash,


The Set will become unnecessary after turning the parent field into TypeList.

radeksimko · 2018-01-08T10:45:30Z

aws/resource_aws_codebuild_project.go

+	values := map[string]interface{}{}
+
+	if cache.Type != nil {
+		if *cache.Type == "NO_CACHE" {


Have you considered making NO_CACHE the Default value of type field instead of "magically" turning it into empty string here?

I'm not sure where's the balance between hiding API's implementation details and helping the user to be honest - just food for thought.

Edit: the code for my first step was wrong, as I didn't use make initially

I wasn't aware of Default so I haven't even tried to go this route. But I do think it sounds a lot cleaner and transparent than what I came up with.

I gave it a go just now, however I couldn't make it work. I can't set Default for only the type, since type is required when cache is present. My next try was to give a Default to the whole cache block, but it won't allow me since it's a list. I figured in this case I should use DefaultFunc instead, however I can't make it work. I came up with

func defaultAwsCodeBuildProjectCache() (interface{}, error) { values := map[string]interface{}{} values["type"] = "NO_CACHE" return []interface{}{values}, nil }

and I was getting Error: aws_codebuild_project.foo: cache.0: expected object, got invalid when trying to generate a plan containing a codebuild project with no cache defined. So I guessed maybe I need to allocate those objects since the might have been gc'ed or something like, so I tried:

func defaultAwsCodeBuildProjectCache() (interface{}, error) { defaultValue := make([]map[string]interface{}, 1) defaultValue[0] = make(map[string]interface{}, 1) defaultValue[0]["type"] = "NO_CACHE" return defaultValue, nil }

But I'm still getting the same error as above. Any ideas on what might be wrong? Please bear in mind that this is my first real contact with Go whatsoever, so I might be stuck in something very basic :-)

I gave it another try yesterday and came to the conclusion that it doesn't make sense to pursue this as a default, since "NO_CACHE" is not a user facing value that is going to be used in HCL. The behaviour of Codebuild projects in the AWS side is a bit weird in the sense that if you don't specify Cache you get a project without caching enabled, while once you set a caching option, the only way to remove it is by updating the project with "NO_CACHE" set. I don't think it makes sense to ask from the user that, once they enabled cached for their Codebuild project on Terraform, they must apply a configuration with "NO_CACHE" explicitly set in order for caching to be disabled. If I remove the cache block from my aws_codebuild_project I wouldn't expect for caching to still be enabled.

If anyone is waiting on me to merge, you can go ahead since I'm done with it unless anyone has some more feedback about it :-)

r/aws_codebuild_project: Add cache options r/aws_codebuild_project: Add documentation cache option r/aws_codebuild_project: Do not display NO_CACHE value in the plan r/aws_codebuild_project: Fix acceptance test r/aws_codebuild_project: Fix indentation in code samples r/aws_codebuild_project: Move cache type validator to validators.go r/aws_codebuild_project: Make cache option into a *schema.List instead of *schema.Set

nathanielks · 2018-02-21T18:55:23Z

@kaofelix There are merge conflicts present, would you mind patching those up? Looking forward to this!

kaofelix · 2018-02-22T07:46:46Z

@nathanielks I didn’t have time to do it yesterday, but I’ll do it later today. Thanks for the heads up!

TaiSHiNet · 2018-02-27T21:35:12Z

@nathanielks are we go with this one?

nathanielks · 2018-02-27T21:36:31Z

@TaiSHiNet unfortunately I'm not a core contributor 😅 just helping where I can!

byF · 2018-03-15T12:23:46Z

Any plans to finish this @radeksimko @nathanielks ? :)

nathanielks · 2018-03-15T17:23:50Z

I'm anxiously awaiting this myself!

kaofelix · 2018-03-15T19:30:25Z

I'm solving the conflicts again, also hoping it gets merged soon

barryoneill · 2018-03-28T20:53:09Z

It'd be awesome if someone would review/merge this - I've got some big ivy builds begging for a cache :)

bflad

Thanks so much for this contribution. Please see the below comments and let me know if you have any questions or do not have time to implement the feedback.

bflad · 2018-04-05T19:38:13Z

aws/validators.go

@@ -15,6 +15,8 @@ import (
 	"github.com/aws/aws-sdk-go/service/cognitoidentityprovider"
 	"github.com/aws/aws-sdk-go/service/configservice"
 	"github.com/aws/aws-sdk-go/service/ec2"
+	"github.com/aws/aws-sdk-go/service/gamelift"


Looks like there was some wonky behavior from merge conflicts. I think the validation functions for gamelift/guardduty are not present in this file in master.

bflad · 2018-04-05T19:45:58Z

aws/resource_aws_codebuild_project_test.go

@@ -459,6 +464,11 @@ resource "aws_codebuild_project" "foo" {
    type = "NO_ARTIFACTS"
  }

+  cache {


Rather than modifying the baseline configuration, we should probably create another acceptance test and configuration(s) that implements these steps:

Create project without the cache configuration

Adding the cache configuration to the project

Updates the location

Removes the cache configuration from the project

bflad · 2018-04-05T19:48:50Z

aws/resource_aws_codebuild_project.go

@@ -59,6 +59,24 @@ func resourceAwsCodeBuildProject() *schema.Resource {
 				},
 				Set: resourceAwsCodeBuildProjectArtifactsHash,
 			},
+			"cache": {


This parent attribute needs to be set as Computed: true due to some quirks working with default values in children attributes. Otherwise this shows up as a perpetual difference for anyone who does not define the cache configuration (also failing the acceptance testing):

=== RUN TestAccAWSCodeBuildProject_default_build_timeout --- FAIL: TestAccAWSCodeBuildProject_default_build_timeout (24.72s) testing.go:518: Step 0 error: After applying this step, the plan was not empty: DIFF: UPDATE: aws_codebuild_project.foo cache.#: "1" => "0" === RUN TestAccAWSCodeBuildProject_sourceAuth --- FAIL: TestAccAWSCodeBuildProject_sourceAuth (24.39s) testing.go:518: Step 1 error: After applying this step, the plan was not empty: DIFF: UPDATE: aws_codebuild_project.foo cache.#: "1" => "0"

hi @bflad! Thanks for your very detailed and helpful feedback. I think I've addressed all of your comments. The new acceptance test I wrote also showed me the effect of lacking the Computed flag you mentioned.

If you have some time, would you mind explaining in a little more detail what this flag is actually doing? I know if I faced this error on my own I would never be able to guess that I should use it, but also if I face something similar in the future I don't wanna feel I'm just cargo culting my way out of it.

Sorry I do not have time to give a full and well thought out response, but the short of it is that Computed tells Terraform to ignore changes to the attribute if it is undefined in the Terraform configuration. It has a side effect of meaning that Terraform cannot reflect drift of a configuration from a default to the operator. Its necessary in this case because we are setting a default for the type child attribute where we allow the operator to not define the configuration to not break backwards compatibility.

A better implementation here would be using CustomizeDiff to manage the nested attributes so we can properly show drift (e.g. someone enabling S3 caching outside TF, but it not being defined in their configuration). In the interest of not holding up this PR longer, I am going to merge this PR as is. We can remove the Computed later.

We will be releasing some public documentation about provider development shortly that will hopefully clear up the usage and caveats with this flag.

bflad · 2018-04-05T19:51:45Z

aws/resource_aws_codebuild_project.go

+						"type": {
+							Type:         schema.TypeString,
+							Required:     true,
+							ValidateFunc: validateAwsCodeBuildCacheType,


Nitpick: This can be simplified with:

ValidateFunc: validation.StringInSlice([]string{ codebuild.CacheTypeNoCache, codebuild.CacheTypeS3, }, false),

…vider-aws

…t by bad rebase

…ate cache type

…t cache

bflad

Merging, thanks! 🚀

Please note that we are pulling this in with the caveat it will not properly detect if S3 cache is already enabled and the cache configuration is not defined (Computed on that attribute), which can be fixed with CustomizeDiff later or improvements into Terraform core to work better with defaults for children attributes.

5 tests passed (all tests)
=== RUN   TestAccAWSCodeBuildProject_basic
--- PASS: TestAccAWSCodeBuildProject_basic (26.63s)
=== RUN   TestAccAWSCodeBuildProject_sourceAuth
--- PASS: TestAccAWSCodeBuildProject_sourceAuth (26.67s)
=== RUN   TestAccAWSCodeBuildProject_default_build_timeout
--- PASS: TestAccAWSCodeBuildProject_default_build_timeout (33.63s)
=== RUN   TestAccAWSCodeBuildProject_vpc
--- PASS: TestAccAWSCodeBuildProject_vpc (43.00s)
=== RUN   TestAccAWSCodeBuildProject_cache
--- PASS: TestAccAWSCodeBuildProject_cache (46.94s)

bflad · 2018-04-06T19:03:15Z

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

nathanielks · 2018-04-06T20:18:51Z

Woohoo!!

TaiSHiNet · 2018-04-07T00:51:28Z

Cheers! Thanks a lot!

ebarault · 2018-04-07T10:30:47Z

Many thanks @kaofelix for your efforts ! 🎉

catsby · 2018-04-09T16:49:01Z

Hi there! I'm afraid we need to revisit this NO_CACHE default idea. See #4113 .

since "NO_CACHE" is not a user facing value that is going to be used in HCL

I don't agree here, because...

If I remove the cache block from my aws_codebuild_project I wouldn't expect for caching to still be enabled.

Unfortunately cache is an Optional + Computed + MaxItems: 1 attribute, and tricky trifecta, which means if you apply with it and then subsequently remove it, Terraform cannot distinguish between "remove this thing" from "I no longer need to care about tracking it's changes". Terraform then doesn't prompt remove the attribute. Assuming I'm reading the code correctly....

bflad · 2018-04-09T22:59:29Z

Followup PR submitted: #4134

ghost · 2020-04-06T17:53:54Z

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!

kaofelix mentioned this pull request Jan 4, 2018

Feature Request: Caching for AWS CodeBuild projects #2842

Closed

Ninir added the enhancement Requests to existing resources that expand the functionality or scope. label Jan 4, 2018

radeksimko reviewed Jan 8, 2018

View reviewed changes

radeksimko added the waiting-response Maintainers are waiting on response from community or contributor. label Jan 8, 2018

kaofelix force-pushed the master branch from f953136 to 534d48e Compare January 8, 2018 14:02

radeksimko removed the waiting-response Maintainers are waiting on response from community or contributor. label Jan 8, 2018

bflad added the service/codebuild Issues and PRs that pertain to the codebuild service. label Jan 11, 2018

kaofelix force-pushed the master branch from 534d48e to 266d1b3 Compare January 12, 2018 13:07

kaofelix force-pushed the master branch from 266d1b3 to 2fbe6d7 Compare February 22, 2018 13:07

ghost added the size/L Managed by automation to categorize the size of a PR. label Feb 22, 2018

kaofelix force-pushed the master branch from 2fbe6d7 to 2dcaec7 Compare February 24, 2018 12:06

ghost added the size/L Managed by automation to categorize the size of a PR. label Feb 24, 2018

kaofelix added 5 commits March 15, 2018 19:53

r/aws_codebuild_project: Add cache options

75db1c4

r/aws_codebuild_project: Add documentation cache option

27d6221

r/aws_codebuild_project: Do not display NO_CACHE value in the plan

8246aff

r/aws_codebuild_project: Fix acceptance test

b3a3bf4

r/aws_codebuild_project: Fix indentation in code samples

ec70fe1

ghost added the size/L Managed by automation to categorize the size of a PR. label Mar 15, 2018

bflad suggested changes Apr 5, 2018

View reviewed changes

bflad added the waiting-response Maintainers are waiting on response from community or contributor. label Apr 5, 2018

bflad self-assigned this Apr 5, 2018

kaofelix added 4 commits April 6, 2018 13:21

Merge branch 'master' of github.com:terraform-providers/terraform-pro…

16e0b42

…vider-aws

validators.go: remove gamelift / guardduty functions added by acciden…

28a1404

…t by bad rebase

resource/aws_codebuild_project: Use validation.StringInSlice to valid…

69f1a23

…ate cache type

resource/aws_codebuild_project: acceptance tests for codebuild projec…

1516c4e

…t cache

ghost added the size/L Managed by automation to categorize the size of a PR. label Apr 6, 2018

bflad self-requested a review April 6, 2018 15:19

bflad removed the waiting-response Maintainers are waiting on response from community or contributor. label Apr 6, 2018

bflad approved these changes Apr 6, 2018

View reviewed changes

bflad merged commit d284993 into hashicorp:master Apr 6, 2018

bflad added this to the v1.14.0 milestone Apr 6, 2018

bflad mentioned this pull request Apr 6, 2018

Add support for dependency caching in AWS Codebuild #2401

Closed

bflad added a commit that referenced this pull request Apr 6, 2018

Update CHANGELOG for #2860

84353e4

catsby mentioned this pull request Apr 9, 2018

aws_codebuild_project: cache config does not stick when cache type set to "NO_CACHE" #4113

Closed

catsby added the waiting-response Maintainers are waiting on response from community or contributor. label Apr 9, 2018

bflad mentioned this pull request Apr 9, 2018

resource/aws_codebuild_project: Properly handle setting cache type NO_CACHE #4134

Merged

ghost locked and limited conversation to collaborators Apr 6, 2020

breathingdust removed the waiting-response Maintainers are waiting on response from community or contributor. label Sep 17, 2021

Add cache option to AWS CodeBuild projects #2860

Add cache option to AWS CodeBuild projects #2860

Conversation

kaofelix commented Jan 4, 2018

nathanielks commented Jan 6, 2018

kaofelix commented Jan 6, 2018

nathanielks commented Jan 7, 2018

nathanielks commented Jan 7, 2018

kaofelix commented Jan 8, 2018

radeksimko left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kaofelix Jan 8, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nathanielks commented Feb 21, 2018

kaofelix commented Feb 22, 2018

TaiSHiNet commented Feb 27, 2018

nathanielks commented Feb 27, 2018

byF commented Mar 15, 2018

nathanielks commented Mar 15, 2018

kaofelix commented Mar 15, 2018

barryoneill commented Mar 28, 2018 • edited Loading

bflad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bflad left a comment

Choose a reason for hiding this comment

bflad commented Apr 6, 2018

nathanielks commented Apr 6, 2018

TaiSHiNet commented Apr 7, 2018

ebarault commented Apr 7, 2018

catsby commented Apr 9, 2018

bflad commented Apr 9, 2018

ghost commented Apr 6, 2020

radeksimko left a comment •

edited

Loading

kaofelix Jan 8, 2018 •

edited

Loading

barryoneill commented Mar 28, 2018 •

edited

Loading