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

feat: Add App Config Feature Flag type #23719

Merged
merged 11 commits into from
Mar 17, 2022
Merged

Conversation

ryancormack
Copy link
Contributor

@ryancormack ryancormack commented Mar 16, 2022

Feature Flags in App Config are now GA, https://aws.amazon.com/about-aws/whats-new/2022/03/aws-appconfig-feature-flags/

This Pull Request exposes the ability to set the Configuration Profile type to either Free Form, or Feature Flag, as per the AWS API docs, https://docs.aws.amazon.com/appconfig/2019-10-09/APIReference/API_CreateConfigurationProfile.html#appconfig-CreateConfigurationProfile-request-Type

I've marked the default value to be FreeForm. Whilst this isn't explicitly documented on AWS, this is the observed behaviour from creating an App Config Profile without setting this value. Using this default maintains backwards compatibility. The validation function uses the regex function documented by AWS

Community Note

  • Please vote on this pull request by adding a 👍 reaction to the original pull request comment to help the community and maintainers prioritize this request
  • Please do not leave "+1" or other comments that do not add relevant new information or questions, they generate extra noise for pull request followers and do not help prioritize the request

Closes #23720

Output from acceptance testing:

$ make testacc TESTS=TestAccXXX PKG=ec2

...

Ryan Cormack added 2 commits March 16, 2022 20:44
Feature Flags are now GA on AWS. Extend the Configuration Profile to expose the ability to set these values.
@github-actions github-actions bot added needs-triage Waiting for first response or review from a maintainer. service/appconfig Issues and PRs that pertain to the appconfig service. size/XS Managed by automation to categorize the size of a PR. labels Mar 16, 2022
@ryancormack
Copy link
Contributor Author

I think this change would satisfy #23720

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Welcome @ryancormack 👋

It looks like this is your first Pull Request submission to the Terraform AWS Provider! If you haven’t already done so please make sure you have checked out our CONTRIBUTING guide and FAQ to make sure your contribution is adhering to best practice and has all the necessary elements in place for a successful approval.

Also take a look at our FAQ which details how we prioritize Pull Requests for inclusion.

Thanks again, and welcome to the community! 😃

@ryancormack ryancormack marked this pull request as ready for review March 16, 2022 21:15
@github-actions github-actions bot added the documentation Introduces or discusses updates to documentation. label Mar 16, 2022
@ryancormack
Copy link
Contributor Author

This is my first contribution to Terraform. I think I've ticked all the contributing requirements. If I've not got something quite right, I'm more than happy to fix a mistake. This is also my first time writing Go. The PR seems small, and I've followed the existing work.

My only thought/consideration was using the default. This is the existing behaviour that happens on the AWS API. I'm unsure whether I should be exposing the default, or leaving it empty, and allowing the AWS API to take care of applying a default.

Thanks

@justinretzolk justinretzolk added enhancement Requests to existing resources that expand the functionality or scope. and removed needs-triage Waiting for first response or review from a maintainer. labels Mar 16, 2022
@github-actions github-actions bot added size/S Managed by automation to categorize the size of a PR. tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure. and removed size/XS Managed by automation to categorize the size of a PR. labels Mar 17, 2022
@ewbankkit
Copy link
Contributor

@ryancormack Thanks for the contribution 🎉 👏.
In general the changes were great.
In order to get this PR merged for this week's release I went ahead and made a few changes:

  • Changed type to ForceNew as it can't be updated in-place
  • Set the type value in the resource Read method
  • Added a local "enumeration" for the valid type values

ewbankkit
ewbankkit previously approved these changes Mar 17, 2022
Copy link
Contributor

@ewbankkit ewbankkit left a comment

Choose a reason for hiding this comment

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

LGTM 🚀.

% make testacc TESTS=TestAccAppConfigConfigurationProfile_ PKG=appconfig ACCTEST_PARALLELISM=3
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./internal/service/appconfig/... -v -count 1 -parallel 3 -run='TestAccAppConfigConfigurationProfile_'  -timeout 180m
=== RUN   TestAccAppConfigConfigurationProfile_basic
=== PAUSE TestAccAppConfigConfigurationProfile_basic
=== RUN   TestAccAppConfigConfigurationProfile_disappears
=== PAUSE TestAccAppConfigConfigurationProfile_disappears
=== RUN   TestAccAppConfigConfigurationProfile_Validators_json
=== PAUSE TestAccAppConfigConfigurationProfile_Validators_json
=== RUN   TestAccAppConfigConfigurationProfile_Validators_lambda
=== PAUSE TestAccAppConfigConfigurationProfile_Validators_lambda
=== RUN   TestAccAppConfigConfigurationProfile_Validators_multiple
=== PAUSE TestAccAppConfigConfigurationProfile_Validators_multiple
=== RUN   TestAccAppConfigConfigurationProfile_updateName
=== PAUSE TestAccAppConfigConfigurationProfile_updateName
=== RUN   TestAccAppConfigConfigurationProfile_updateDescription
=== PAUSE TestAccAppConfigConfigurationProfile_updateDescription
=== RUN   TestAccAppConfigConfigurationProfile_tags
=== PAUSE TestAccAppConfigConfigurationProfile_tags
=== CONT  TestAccAppConfigConfigurationProfile_basic
=== CONT  TestAccAppConfigConfigurationProfile_Validators_multiple
=== CONT  TestAccAppConfigConfigurationProfile_tags
--- PASS: TestAccAppConfigConfigurationProfile_basic (19.45s)
=== CONT  TestAccAppConfigConfigurationProfile_updateDescription
--- PASS: TestAccAppConfigConfigurationProfile_tags (44.72s)
=== CONT  TestAccAppConfigConfigurationProfile_updateName
--- PASS: TestAccAppConfigConfigurationProfile_Validators_multiple (47.92s)
=== CONT  TestAccAppConfigConfigurationProfile_Validators_json
--- PASS: TestAccAppConfigConfigurationProfile_updateDescription (31.96s)
=== CONT  TestAccAppConfigConfigurationProfile_Validators_lambda
--- PASS: TestAccAppConfigConfigurationProfile_updateName (29.24s)
=== CONT  TestAccAppConfigConfigurationProfile_disappears
--- PASS: TestAccAppConfigConfigurationProfile_disappears (15.54s)
--- PASS: TestAccAppConfigConfigurationProfile_Validators_json (42.86s)
--- PASS: TestAccAppConfigConfigurationProfile_Validators_lambda (78.26s)
PASS
ok  	github.com/hashicorp/terraform-provider-aws/internal/service/appconfig	134.004s

@ryancormack
Copy link
Contributor Author

Thanks @ewbankkit .

I'm just testing the various behaviours with this App Config option locally now. I'm discovering some 'quirks' with the API. Some of the AWS docs don't appear to be fully complete, so I want to make sure I'm not doing anything unintended.

Should I close the PR for this and re-open as a draft?

@ewbankkit ewbankkit self-requested a review March 17, 2022 12:53
@ewbankkit ewbankkit dismissed their stale review March 17, 2022 12:54

Contributor want to make more changes

@ewbankkit
Copy link
Contributor

@ryancormack You can use this PR to make additional changes; I dismissed the Approval.

Show example differences for freeform and feature flags hosted configuration profile types
@ryancormack
Copy link
Contributor Author

@ewbankkit The behaviour I was unsure about was fixed by your change to enable ForceNew - learning more new things there.

I've added some documentation to show example schemas for the two types of configuration profile versions now too.

I think it's finished from me and is behaving as intended after that ForceNew.

Thanks very much

@github-actions github-actions bot removed the size/S Managed by automation to categorize the size of a PR. label Mar 17, 2022
@github-actions github-actions bot added the size/M Managed by automation to categorize the size of a PR. label Mar 17, 2022
@ewbankkit ewbankkit merged commit eadb7f4 into hashicorp:main Mar 17, 2022
@github-actions github-actions bot added this to the v4.6.0 milestone Mar 17, 2022
@ryancormack
Copy link
Contributor Author

Thanks very much for the help and tidy up Kit 👍🏻

@github-actions
Copy link

This functionality has been released in v4.6.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. Thank you!

@github-actions
Copy link

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 10, 2022
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/appconfig Issues and PRs that pertain to the appconfig service. size/M 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.

Expose App Config Configuration Profile Types
3 participants