-
Notifications
You must be signed in to change notification settings - Fork 9.2k
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
Conversation
@@ -134,6 +134,7 @@ The following arguments are supported: | |||
`environment_variable` supports the following: |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
Please Do! |
There was a problem hiding this 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 | ||
}, |
There was a problem hiding this comment.
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 Any chance this can go into the next release? (and thank you @atsushi-ishibashi !) |
Also includes @atsushi-ishibashi fix from hashicorp#2811
(incase @atsushi-ishibashi doesn't have time to fix the doc thing, I did a PR with just that in, and his change: #3323 ) |
if env.Type != nil {
item["type"] = *env.Type
} |
* add in new line so it parses properly Also includes @atsushi-ishibashi fix from #2811 * remove the type, which can be added back later
* add in new line so it parses properly Also includes @atsushi-ishibashi fix from #2811 * remove the type, which can be added back later
|
@bflad / @catsby - looks like @atsushi-ishibashi has added the |
For the acceptance testing, we'd normally expect a If you don't have time to submit these fixes, please let us know and we can finish them up. 👍 |
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. |
I made a PR with fix for tests. Currently this PR is marked as WIP because I have to resolve some conflicts. |
@larsjarek Hey thanks for this. Looking forward to that getting merged. |
Sorry for replying late. I have no time to update so I'm glad if anyone take this PR over:bow: |
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. |
when this will be merged I need this for a project I'm working on ? |
It does not seem that this feature will be merged soon. |
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:
|
After merging together the above pull requests and refactoring the test suite, we should be good to go:
This will release with v1.21.0 of the AWS provider, likely middle of next week. |
@bflad Thanks a lot for merging this. |
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. |
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! |
Supported: #1747