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(parameters): migrate AppConfig to new APIs due to API deprecation #1553

Merged

Conversation

leandrodamascena
Copy link
Contributor

@leandrodamascena leandrodamascena commented Sep 29, 2022

Issue number: #1506

Summary

Changes

Due to Deprecation of the AppConfig API, we needed to change the SDK client to call the new APIs.

We had a long discussion in this issue #1506 before applying these changes.

User experience

No changes are required to Lambda functions that use Feature Flags and AppConfig Parameter utility, but users must add appconfig:GetLatestConfiguration and appconfig:StartConfigurationSession IAM permissions for this to work in their Lambdas.

Checklist

If your change doesn't seem to apply, please leave them unchecked.

Is this a breaking change?

RFC issue number:

Checklist:

  • Migration process documented
  • Implement warnings (if it can live side by side)

Acknowledgment

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

Disclaimer: We value your time and bandwidth. As such, any pull requests created on non-triaged issues might not be successful.


View rendered docs/upgrade.md

@leandrodamascena leandrodamascena requested a review from a team as a code owner September 29, 2022 12:55
@leandrodamascena leandrodamascena requested review from am29d and removed request for a team September 29, 2022 12:55
@boring-cyborg boring-cyborg bot added dependencies Pull requests that update a dependency file documentation Improvements or additions to documentation tests labels Sep 29, 2022
@pull-request-size pull-request-size bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Sep 29, 2022
@github-actions github-actions bot added the feature New feature or functionality label Sep 29, 2022
@leandrodamascena leandrodamascena requested review from heitorlessa and rubenfonseca and removed request for am29d and heitorlessa September 29, 2022 13:01
@pull-request-size pull-request-size bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 1, 2022
@codecov-commenter
Copy link

codecov-commenter commented Oct 1, 2022

Codecov Report

Base: 99.42% // Head: 99.38% // Decreases project coverage by -0.03% ⚠️

Coverage data is based on head (f3abe06) compared to base (fd7c235).
Patch coverage: 87.50% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##               v2    #1553      +/-   ##
==========================================
- Coverage   99.42%   99.38%   -0.04%     
==========================================
  Files         125      125              
  Lines        5726     5731       +5     
  Branches      357      359       +2     
==========================================
+ Hits         5693     5696       +3     
  Misses         18       18              
- Partials       15       17       +2     
Impacted Files Coverage Δ
...ambda_powertools/utilities/parameters/appconfig.py 94.28% <86.66%> (-5.72%) ⬇️
aws_lambda_powertools/utilities/parameters/base.py 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@leandrodamascena leandrodamascena requested review from heitorlessa and removed request for rubenfonseca October 3, 2022 08:22
@leandrodamascena
Copy link
Contributor Author

Giving feedback here.
I need to add more information in the doc files, but we hope to review and merge this PR later this week.

@heitorlessa
Copy link
Contributor

Looking into this now [apologies for the delay, re:Invent deadlines are tighter this year]

Copy link
Contributor

@heitorlessa heitorlessa left a comment

Choose a reason for hiding this comment

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

Thanks a lot @leandrodamascena for working on this, truly!

I've made some wording suggestions, and some concerns on E2E tests:

  • Separate SSM from this PR for a clean cut and revert if we need
  • Some resources have names hardcoded, this might bite us back in parallel testing or network failures (e.g., SSM will fail subsequent stack if a param name already exists)

aws_lambda_powertools/utilities/parameters/appconfig.py Outdated Show resolved Hide resolved
aws_lambda_powertools/utilities/parameters/appconfig.py Outdated Show resolved Hide resolved
aws_lambda_powertools/utilities/parameters/appconfig.py Outdated Show resolved Hide resolved
docs/upgrade.md Outdated Show resolved Hide resolved
docs/upgrade.md Show resolved Hide resolved
tests/e2e/parameters/test_appconfig.py Show resolved Hide resolved
tests/e2e/parameters/infrastructure.py Show resolved Hide resolved
tests/e2e/parameters/infrastructure.py Outdated Show resolved Hide resolved
tests/e2e/parameters/infrastructure.py Outdated Show resolved Hide resolved
tests/e2e/parameters/infrastructure.py Outdated Show resolved Hide resolved
@leandrodamascena
Copy link
Contributor Author

leandrodamascena commented Oct 14, 2022

Thanks a lot @leandrodamascena for working on this, truly!

I've made some wording suggestions, and some concerns on E2E tests:

  • Separate SSM from this PR for a clean cut and revert if we need
  • Some resources have names hardcoded, this might bite us back in parallel testing or network failures (e.g., SSM will fail subsequent stack if a param name already exists)

Heiiiiii @heitorlessa! I've addressed all the feedback and I think we're ready to merge.

Thank you so much for reviewing this, every time you give me WONDERFUL feedback to think about UX and see beyond the code. ❤️

image

Copy link
Contributor

@heitorlessa heitorlessa left a comment

Choose a reason for hiding this comment

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

THANK YOU! I'm glad we're completing this and we're ready for the grooming part before launching V2 now.

Last suggestions I'm committing myself is on being more explicit on AppConfig Config/Env Name and Description. If a network split happens, this will leave dangling resources making it difficult to trace back to what they are for (appe2e vs powertools-e2e-x).

docs/utilities/feature_flags.md Outdated Show resolved Hide resolved
tests/e2e/parameters/infrastructure.py Outdated Show resolved Hide resolved
tests/e2e/parameters/infrastructure.py Outdated Show resolved Hide resolved
tests/e2e/parameters/infrastructure.py Outdated Show resolved Hide resolved
@heitorlessa heitorlessa changed the title feat(appconfigdata): Change parameters appconfig utility API due to GetConfiguration deprecation feat(parameters): migrate AppConfig to new APIs due to API deprecation Oct 14, 2022
@leandrodamascena leandrodamascena merged commit 4b76eca into aws-powertools:v2 Oct 14, 2022
@leandrodamascena leandrodamascena deleted the feat/new-appconfig-apis branch October 14, 2022 16:38
rubenfonseca pushed a commit that referenced this pull request Oct 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file documentation Improvements or additions to documentation feature New feature or functionality size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Maintenance: Change parameters appconfig utility API due to GetConfiguration deprecation
4 participants