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

r/codebuild_project: Support type for environment_variable #2811

Merged
merged 3 commits into from
May 24, 2018

Conversation

atsushi-ishibashi
Copy link
Contributor

@atsushi-ishibashi atsushi-ishibashi commented Dec 31, 2017

Supported: #1747

  • acceptance test

@radeksimko radeksimko added the enhancement Requests to existing resources that expand the functionality or scope. label Jan 2, 2018
@bflad bflad added the service/codebuild Issues and PRs that pertain to the codebuild service. label Jan 28, 2018
@@ -134,6 +134,7 @@ The following arguments are supported:
`environment_variable` supports the following:
Copy link
Contributor

Choose a reason for hiding this comment

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

Any chance you can add an extra CR at the end of this? the docs don't parse properly otherwise.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is done and merged BTW

@nicwise
Copy link
Contributor

nicwise commented Feb 8, 2018

Please Do!

Copy link
Member

@radeksimko radeksimko left a comment

Choose a reason for hiding this comment

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

Just one nitpick about validation, but can you also add an acceptance test which involves this argument and (presumably) SSM parameter?

}
errors = append(errors, fmt.Errorf("expected %s to be one of %v, got %s", k, validType, value))
return
},
Copy link
Member

Choose a reason for hiding this comment

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

Nitpick: Do you mind replacing this with a standard helper we have in the upstream library and constants from the SDK? i.e.

ValidateFunc: validation.StringInSlice([]string{
  codebuild.EnvironmentVariableTypeParameterStore,
  codebuild.EnvironmentVariableTypePlaintext,
}, true),

@radeksimko radeksimko added the waiting-response Maintainers are waiting on response from community or contributor. label Feb 8, 2018
@ghost ghost added the size/S Managed by automation to categorize the size of a PR. label Feb 10, 2018
@nicwise
Copy link
Contributor

nicwise commented Feb 10, 2018

@radeksimko Any chance this can go into the next release? (and thank you @atsushi-ishibashi !)

nicwise added a commit to nicwise/terraform-provider-aws that referenced this pull request Feb 10, 2018
@nicwise
Copy link
Contributor

nicwise commented Feb 10, 2018

(incase @atsushi-ishibashi doesn't have time to fix the doc thing, I did a PR with just that in, and his change: #3323 )

@bflad
Copy link
Contributor

bflad commented Feb 12, 2018

environmentVariablesToMap will likely also need an update so the new optional attribute properly gets read (the missing acceptance test would've caught this):

if env.Type != nil {
  item["type"] = *env.Type
}

catsby pushed a commit that referenced this pull request Feb 13, 2018
* add in new line so it parses properly

Also includes @atsushi-ishibashi fix from #2811

* remove the type, which can be added back later
catsby pushed a commit that referenced this pull request Feb 13, 2018
* add in new line so it parses properly

Also includes @atsushi-ishibashi fix from #2811

* remove the type, which can be added back later
@ghost ghost added the size/S Managed by automation to categorize the size of a PR. label Feb 23, 2018
@atsushi-ishibashi
Copy link
Contributor Author

TF_ACC=1 go test ./aws -v -run=TestAccAWSCodeBuildProject_basic -timeout 120m
=== RUN   TestAccAWSCodeBuildProject_basic
--- PASS: TestAccAWSCodeBuildProject_basic (96.77s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	96.810s

@nicwise
Copy link
Contributor

nicwise commented Feb 28, 2018

@bflad / @catsby - looks like @atsushi-ishibashi has added the environmentVariablesToMap thing - is this one good to go? ;-)

@bflad
Copy link
Contributor

bflad commented Feb 28, 2018

For the acceptance testing, we'd normally expect a _basic test to not include an optional attribute (duplicating the entire test and config is fine) and for the test including a new attribute to at least check the attribute value in the Terraform state via resource.TestCheckResourceAttr(resourceName, "attribute", "expectedValue").

If you don't have time to submit these fixes, please let us know and we can finish them up. 👍

@larsjarek
Copy link

Hey guys, is there a chance that this feature can go into the next release? I would really appreciate it as it is needed for my AWS CodeBuild configuration. If @atsushi-ishibashi does not have time I might fix these tests in another PR.

@larsjarek
Copy link

I made a PR with fix for tests. Currently this PR is marked as WIP because I have to resolve some conflicts.

@kathgironpe
Copy link

@larsjarek Hey thanks for this. Looking forward to that getting merged.

@atsushi-ishibashi
Copy link
Contributor Author

Sorry for replying late. I have no time to update so I'm glad if anyone take this PR over:bow:

@larsjarek
Copy link

No problem @atsushi-ishibashi, thanks for your contribution up to this point. All conflicts in my PR (#4021) are now resolved, it can be reviewed now.

@bnightstar
Copy link

when this will be merged I need this for a project I'm working on ?

@larsjarek
Copy link

It does not seem that this feature will be merged soon.

@bflad
Copy link
Contributor

bflad commented May 24, 2018

The later #4021 (containing this PR) is actively being reviewed this week.

edit: #3929

@bflad
Copy link
Contributor

bflad commented May 24, 2018

After looking through the various duplicate pull requests, I'm going to try to get the commits merged in the following order here (resolving the merge conflicts and test changes along the way, which could be tedious): #2811 -> #4021 -> #3929. 🤞

In the end the following new arguments will be supported:

  • environment > environment_variable > type
  • source > git_clone_depth
  • source > insecure_ssl

@bflad
Copy link
Contributor

bflad commented May 24, 2018

After merging together the above pull requests and refactoring the test suite, we should be good to go:

make testacc TEST=./aws TESTARGS='-run=TestAccAWSCodeBuildProject'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -run=TestAccAWSCodeBuildProject -timeout 120m
=== RUN   TestAccAWSCodeBuildProject_basic
--- PASS: TestAccAWSCodeBuildProject_basic (31.87s)
=== RUN   TestAccAWSCodeBuildProject_BadgeEnabled
--- PASS: TestAccAWSCodeBuildProject_BadgeEnabled (30.05s)
=== RUN   TestAccAWSCodeBuildProject_BuildTimeout
--- PASS: TestAccAWSCodeBuildProject_BuildTimeout (38.35s)
=== RUN   TestAccAWSCodeBuildProject_Cache
--- PASS: TestAccAWSCodeBuildProject_Cache (59.77s)
=== RUN   TestAccAWSCodeBuildProject_Description
--- PASS: TestAccAWSCodeBuildProject_Description (38.08s)
=== RUN   TestAccAWSCodeBuildProject_EncryptionKey
--- PASS: TestAccAWSCodeBuildProject_EncryptionKey (53.94s)
=== RUN   TestAccAWSCodeBuildProject_Environment_EnvironmentVariable_Type
--- PASS: TestAccAWSCodeBuildProject_Environment_EnvironmentVariable_Type (30.31s)
=== RUN   TestAccAWSCodeBuildProject_Source_Auth
--- PASS: TestAccAWSCodeBuildProject_Source_Auth (20.33s)
=== RUN   TestAccAWSCodeBuildProject_Source_GitCloneDepth
--- PASS: TestAccAWSCodeBuildProject_Source_GitCloneDepth (38.51s)
=== RUN   TestAccAWSCodeBuildProject_Source_InsecureSSL
--- PASS: TestAccAWSCodeBuildProject_Source_InsecureSSL (37.43s)
=== RUN   TestAccAWSCodeBuildProject_Tags
--- PASS: TestAccAWSCodeBuildProject_Tags (28.91s)
=== RUN   TestAccAWSCodeBuildProject_VpcConfig
--- PASS: TestAccAWSCodeBuildProject_VpcConfig (57.63s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	465.209s

This will release with v1.21.0 of the AWS provider, likely middle of next week.

@bflad bflad added this to the v1.21.0 milestone May 24, 2018
@bflad bflad merged commit 7999b0c into hashicorp:master May 24, 2018
bflad added a commit that referenced this pull request May 24, 2018
@larsjarek
Copy link

larsjarek commented May 28, 2018

@bflad Thanks a lot for merging this.

@bflad
Copy link
Contributor

bflad commented May 31, 2018

This has been released in version 1.21.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 5, 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 5, 2020
@breathingdust breathingdust removed the waiting-response Maintainers are waiting on response from community or contributor. label Sep 17, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement Requests to existing resources that expand the functionality or scope. service/codebuild Issues and PRs that pertain to the codebuild service. size/S Managed by automation to categorize the size of a PR.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants