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.

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

Add cache option to AWS CodeBuild projects #2860

Merged
merged 11 commits into from
Apr 6, 2018
Merged

Add cache option to AWS CodeBuild projects #2860

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
75db1c4
r/aws_codebuild_project: Add cache options
kaofelix Jan 3, 2018
27d6221
r/aws_codebuild_project: Add documentation cache option
kaofelix Jan 4, 2018
8246aff
r/aws_codebuild_project: Do not display NO_CACHE value in the plan
kaofelix Jan 4, 2018
b3a3bf4
r/aws_codebuild_project: Fix acceptance test
kaofelix Jan 4, 2018
ec70fe1
r/aws_codebuild_project: Fix indentation in code samples
kaofelix Jan 4, 2018
536dea9
r/aws_codebuild_project: Move cache type validator to validators.go
kaofelix Jan 8, 2018
0090b9f
r/aws_codebuild_project: Make cache option into a *schema.List instea…
kaofelix Jan 8, 2018
16e0b42
Merge branch 'master' of github.com:terraform-providers/terraform-pro…
kaofelix Apr 6, 2018
28a1404
validators.go: remove gamelift / guardduty functions added by acciden…
kaofelix Apr 6, 2018
69f1a23
resource/aws_codebuild_project: Use validation.StringInSlice to valid…
kaofelix Apr 6, 2018
1516c4e
resource/aws_codebuild_project: acceptance tests for codebuild projec…
kaofelix Apr 6, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
67 changes: 67 additions & 0 deletions aws/resource_aws_codebuild_project.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,24 @@ func resourceAwsCodeBuildProject() *schema.Resource {
},
Set: resourceAwsCodeBuildProjectArtifactsHash,
},
"cache": {
Copy link
Contributor

Choose a reason for hiding this comment

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

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"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Type: schema.TypeList,
Optional: true,
MaxItems: 1,
Elem: &schema.Resource{
Schema: map[string]*schema.Schema{
"type": {
Type: schema.TypeString,
Required: true,
ValidateFunc: validateAwsCodeBuildCacheType,
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpick: This can be simplified with:

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

},
"location": {
Type: schema.TypeString,
Required: true,
},
},
},
},
"description": {
Type: schema.TypeString,
Optional: true,
Expand Down Expand Up @@ -226,6 +244,10 @@ func resourceAwsCodeBuildProjectCreate(d *schema.ResourceData, meta interface{})
Artifacts: &projectArtifacts,
}

if v, ok := d.GetOk("cache"); ok {
params.Cache = expandProjectCache(v.([]interface{}))
}

if v, ok := d.GetOk("description"); ok {
params.Description = aws.String(v.(string))
}
Expand Down Expand Up @@ -312,6 +334,19 @@ func expandProjectArtifacts(d *schema.ResourceData) codebuild.ProjectArtifacts {
return projectArtifacts
}

func expandProjectCache(s []interface{}) *codebuild.ProjectCache {
var projectCache *codebuild.ProjectCache

data := s[0].(map[string]interface{})

projectCache = &codebuild.ProjectCache{
Type: aws.String(data["type"].(string)),
Location: aws.String(data["location"].(string)),
}

return projectCache
}

func expandProjectEnvironment(d *schema.ResourceData) *codebuild.ProjectEnvironment {
configs := d.Get("environment").(*schema.Set).List()

Expand Down Expand Up @@ -438,6 +473,10 @@ func resourceAwsCodeBuildProjectRead(d *schema.ResourceData, meta interface{}) e
return err
}

if err := d.Set("cache", flattenAwsCodebuildProjectCache(project.Cache)); err != nil {
return err
}

if err := d.Set("source", flattenAwsCodeBuildProjectSource(project.Source)); err != nil {
return err
}
Expand Down Expand Up @@ -485,6 +524,16 @@ func resourceAwsCodeBuildProjectUpdate(d *schema.ResourceData, meta interface{})
params.VpcConfig = expandCodeBuildVpcConfig(d.Get("vpc_config").([]interface{}))
}

if d.HasChange("cache") {
if v, ok := d.GetOk("cache"); ok {
params.Cache = expandProjectCache(v.([]interface{}))
} else {
params.Cache = &codebuild.ProjectCache{
Type: aws.String("NO_CACHE"),
}
}
}

if d.HasChange("description") {
params.Description = aws.String(d.Get("description").(string))
}
Expand Down Expand Up @@ -567,6 +616,24 @@ func flattenAwsCodeBuildProjectArtifacts(artifacts *codebuild.ProjectArtifacts)
return &artifactSet
}

func flattenAwsCodebuildProjectCache(cache *codebuild.ProjectCache) []interface{} {
values := map[string]interface{}{}

if cache.Type != nil {
if *cache.Type == "NO_CACHE" {
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@kaofelix kaofelix Jan 8, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

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 :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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 :-)

values["type"] = ""
} else {
values["type"] = *cache.Type
}
}

if cache.Location != nil {
values["location"] = *cache.Location
}

return []interface{}{values}
}

func flattenAwsCodeBuildProjectEnvironment(environment *codebuild.ProjectEnvironment) []interface{} {
envConfig := map[string]interface{}{}

Expand Down
24 changes: 22 additions & 2 deletions aws/resource_aws_codebuild_project_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -388,6 +388,11 @@ func testAccCheckAWSCodeBuildProjectDestroy(s *terraform.State) error {

func testAccAWSCodeBuildProjectConfig_basic(rName, vpcConfig, vpcResources string) string {
return fmt.Sprintf(`
resource "aws_s3_bucket" "foo" {
bucket = "tf-test-codebuild-%s"
acl = "private"
}

resource "aws_iam_role" "codebuild_role" {
name = "codebuild-role-%s"
assume_role_policy = <<EOF
Expand Down Expand Up @@ -459,6 +464,11 @@ resource "aws_codebuild_project" "foo" {
type = "NO_ARTIFACTS"
}

cache {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

type = "S3"
location = "${aws_s3_bucket.foo.bucket}"
}

environment {
compute_type = "BUILD_GENERAL1_SMALL"
image = "2"
Expand All @@ -481,11 +491,16 @@ resource "aws_codebuild_project" "foo" {
%s
}
%s
`, rName, rName, rName, rName, vpcConfig, vpcResources)
`, rName, rName, rName, rName, rName, vpcConfig, vpcResources)
}

func testAccAWSCodeBuildProjectConfig_basicUpdated(rName string) string {
return fmt.Sprintf(`
resource "aws_s3_bucket" "foo" {
bucket = "tf-test-codebuild-%s"
acl = "private"
}

resource "aws_iam_role" "codebuild_role" {
name = "codebuild-role-%s"
assume_role_policy = <<EOF
Expand Down Expand Up @@ -544,6 +559,11 @@ resource "aws_codebuild_project" "foo" {
type = "NO_ARTIFACTS"
}

cache {
type = "S3"
location = "${aws_s3_bucket.foo.bucket}"
}

environment {
compute_type = "BUILD_GENERAL1_SMALL"
image = "2"
Expand All @@ -564,7 +584,7 @@ resource "aws_codebuild_project" "foo" {
"Environment" = "Test"
}
}
`, rName, rName, rName, rName)
`, rName, rName, rName, rName, rName)
}

func testAccAWSCodeBuildProjectConfig_default_timeout(rName string) string {
Expand Down
65 changes: 65 additions & 0 deletions aws/validators.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

"github.com/aws/aws-sdk-go/service/guardduty"
"github.com/aws/aws-sdk-go/service/s3"
"github.com/aws/aws-sdk-go/service/waf"
"github.com/hashicorp/terraform/helper/schema"
Expand Down Expand Up @@ -1656,6 +1658,69 @@ func validateAwsElastiCacheReplicationGroupAuthToken(v interface{}, k string) (w
return
}

func validateAwsCodeBuildCacheType(v interface{}, k string) (ws []string, errors []error) {
value := v.(string)
types := map[string]bool{
"S3": true,
}

if !types[value] {
errors = append(errors, fmt.Errorf("CodeBuild: Cache Type can only be S3"))
}
return
}

func validateGameliftOperatingSystem(v interface{}, k string) (ws []string, errors []error) {
value := v.(string)
operatingSystems := map[string]bool{
gamelift.OperatingSystemAmazonLinux: true,
gamelift.OperatingSystemWindows2012: true,
}

if !operatingSystems[value] {
errors = append(errors, fmt.Errorf("%q must be a valid operating system value: %q", k, value))
}
return
}

func validateGuardDutyIpsetFormat(v interface{}, k string) (ws []string, errors []error) {
value := v.(string)
validType := []string{
guardduty.IpSetFormatTxt,
guardduty.IpSetFormatStix,
guardduty.IpSetFormatOtxCsv,
guardduty.IpSetFormatAlienVault,
guardduty.IpSetFormatProofPoint,
guardduty.IpSetFormatFireEye,
}
for _, str := range validType {
if value == str {
return
}
}
errors = append(errors, fmt.Errorf("expected %s to be one of %v, got %s", k, validType, value))
return
}

func validateGuardDutyThreatIntelSetFormat(v interface{}, k string) (ws []string, errors []error) {
value := v.(string)
validType := []string{
guardduty.ThreatIntelSetFormatTxt,
guardduty.ThreatIntelSetFormatStix,
guardduty.ThreatIntelSetFormatOtxCsv,
guardduty.ThreatIntelSetFormatAlienVault,
guardduty.ThreatIntelSetFormatProofPoint,
guardduty.ThreatIntelSetFormatFireEye,
}
for _, str := range validType {
if value == str {
return
}
}
errors = append(errors, fmt.Errorf("expected %s to be one of %v, got %s", k, validType, value))
return
}

func validateDynamoDbStreamSpec(d *schema.ResourceDiff) error {
enabled := d.Get("stream_enabled").(bool)
if enabled {
Expand Down
18 changes: 18 additions & 0 deletions aws/validators_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2503,6 +2503,24 @@ func TestValidateCognitoUserPoolDomain(t *testing.T) {
}
}

func TestValidateAwsCodeBuildCacheType(t *testing.T) {
cases := []struct {
Value string
ErrCount int
}{
{Value: "S3", ErrCount: 0},
{Value: "XYZ", ErrCount: 1},
}

for _, tc := range cases {
_, errors := validateAwsCodeBuildCacheType(tc.Value, "aws_codebuild_project")

if len(errors) != tc.ErrCount {
t.Fatalf("Expected the AWS CodeBuild project artifacts type to trigger a validation error")
}
}
}

func TestValidateCognitoUserGroupName(t *testing.T) {
validValues := []string{
"foo",
Expand Down
16 changes: 16 additions & 0 deletions website/docs/r/codebuild_project.html.markdown
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,11 @@ Provides a CodeBuild Project resource.
## Example Usage

```hcl
resource "aws_s3_bucket" "foo" {
bucket = "test-bucket"
acl = "private"
}

resource "aws_iam_role" "codebuild_role" {
name = "codebuild-role-"

Expand Down Expand Up @@ -73,6 +78,11 @@ resource "aws_codebuild_project" "foo" {
type = "NO_ARTIFACTS"
}

cache {
type = "S3"
location = "${aws_s3_bucket.foo.bucket}"
}

environment {
compute_type = "BUILD_GENERAL1_SMALL"
image = "aws/codebuild/nodejs:6.3.1"
Expand Down Expand Up @@ -125,6 +135,7 @@ The following arguments are supported:
* `build_timeout` - (Optional) How long in minutes, from 5 to 480 (8 hours), for AWS CodeBuild to wait until timing out any related build that does not get marked as completed. The default is 60 minutes.
* `tags` - (Optional) A mapping of tags to assign to the resource.
* `artifacts` - (Required) Information about the project's build output artifacts. Artifact blocks are documented below.
* `cache` - (Optional) Information about the cache storage for the project. Cache blocks are documented below.
* `environment` - (Required) Information about the project's build environment. Environment blocks are documented below.
* `source` - (Required) Information about the project's input source code. Source blocks are documented below.
* `vpc_config` - (Optional) Configuration for the builds to run inside a VPC. VPC config blocks are documented below.
Expand All @@ -138,6 +149,11 @@ The following arguments are supported:
* `packaging` - (Optional) The type of build output artifact to create. If `type` is set to `S3`, valid values for this parameter are: `NONE` or `ZIP`
* `path` - (Optional) If `type` is set to `S3`, this is the path to the output artifact

`cache` supports the following:

* `type` - (Required) The type of storage that will be used for the AWS CodeBuild project cache. The only valid value is `S3`.
* `location` - (Required) The location where the AWS CodeBuild project stores cached resources. Has to be an S3 bucket.

`environment` supports the following:

* `compute_type` - (Required) Information about the compute resources the build project will use. Available values for this parameter are: `BUILD_GENERAL1_SMALL`, `BUILD_GENERAL1_MEDIUM` or `BUILD_GENERAL1_LARGE`
Expand Down