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

fix(parameter): improve AppConfig cached configuration retrieval #3195

Merged

Conversation

leandrodamascena
Copy link
Contributor

@leandrodamascena leandrodamascena commented Oct 12, 2023

Issue number: #3194

Summary

Changes

This pull request fix an issue related to AppConfig caching behavior when multiple configuration calls are made using the same application/environment.

The expected behavior of get_latest_configuration function is to return a None/null value when no new configuration is available. This design is intentional and value should be cached on the customer side. We created a dictionary for storing the most recently retrieved value for a specific configuration profile.

For additional context and details regarding the behavior of get_latest_configuration, please refer to the official AWS AppConfig documentation.

User experience

There is no change in the user experience, but it fixes the error.

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.

@leandrodamascena leandrodamascena requested a review from a team as a code owner October 12, 2023 17:59
@pull-request-size pull-request-size bot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Oct 12, 2023
@leandrodamascena leandrodamascena linked an issue Oct 12, 2023 that may be closed by this pull request
@codecov-commenter
Copy link

codecov-commenter commented Oct 12, 2023

Codecov Report

All modified lines are covered by tests ✅

Comparison is base (5bfb60f) 95.96% compared to head (c567ff0) 95.96%.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@           Coverage Diff            @@
##           develop    #3195   +/-   ##
========================================
  Coverage    95.96%   95.96%           
========================================
  Files          195      195           
  Lines         8387     8387           
  Branches      1563     1563           
========================================
  Hits          8049     8049           
  Misses         276      276           
  Partials        62       62           
Files Coverage Δ
...ambda_powertools/utilities/parameters/appconfig.py 94.59% <100.00%> (ø)

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

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.

Tiny changes - LGTM!!

  1. Update PR body - what's the problem of making multiple calls? This will help improve the PR title in the Changelog too

  2. Code comment on why we need to store last returned as a dict

  3. Create an issue to add a E2E test after the release (non-blocking) so we can be confident there's no regression in the future

@leandrodamascena leandrodamascena changed the title fix(parameter): fix AppConfig cache for multiple configurations calls fix(parameter): fix AppConfig caching and get_latest_configuration behavior for multiple configuration calls Oct 13, 2023
@pull-request-size pull-request-size bot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Oct 13, 2023
@leandrodamascena leandrodamascena added bug Something isn't working parameters Parameters utility labels Oct 13, 2023
@leandrodamascena leandrodamascena changed the title fix(parameter): fix AppConfig caching and get_latest_configuration behavior for multiple configuration calls fix(parameter): fix AppConfig caching get_latest_configuration return for multiple configuration calls Oct 13, 2023
@rubenfonseca
Copy link
Contributor

Looking at this now

@rubenfonseca rubenfonseca self-requested a review October 13, 2023 10:10
rubenfonseca
rubenfonseca previously approved these changes Oct 13, 2023
Copy link
Contributor

@rubenfonseca rubenfonseca left a comment

Choose a reason for hiding this comment

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

Just left one tiny thing, but it's ready to go!

aws_lambda_powertools/utilities/parameters/appconfig.py Outdated Show resolved Hide resolved
@rubenfonseca rubenfonseca changed the title fix(parameter): fix AppConfig caching get_latest_configuration return for multiple configuration calls fix(parameter): improve AppConfig cached configuration retrieval Oct 13, 2023
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@leandrodamascena leandrodamascena merged commit 52d54ab into aws-powertools:develop Oct 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working parameters Parameters utility size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug: AppConfig Provider Returning Wrong Configuration
4 participants