-
Notifications
You must be signed in to change notification settings - Fork 4k
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(core): support adding additional cache key to cdk context values #28766
Conversation
This PR continues from #26984, which was closed as stale while waiting for feedback from a maintainer |
This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state. |
1 similar comment
This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state. |
Sorry for the delays. I took a look through the linked PR that was closed and I'm not totally sure I understand @rix0rrr's last comment so I'm going to sync with him before giving this a thorough review. Just wanted to give you a heads up that this is on my radar since I know you've been waiting a while for a new review. Thank you for all the work on this! I'll have some real comments either tomorrow or Monday. |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
Oops didn't mean to merge in latest, but it looks like everything is good there anyways! |
These two PRs (#29372) and (#28766) are not doing the same thing, but there is definitely overlap. Given that fact, it changes the feedback I was going to give to both a bit. I absolutely see the value in both these changes but want to make sure we're getting the contract right across both. I know you've both been waiting for feedback for some time but I'm going to tax your patience a tad further so I can discuss with the team tomorrow. |
To customize the cache key use the `additionalCacheKey` parameter. | ||
This can be useful if you want to scope the context variable to a construct (eg, using `additionalCacheKey: this.node.path`). |
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.
Why would a user want to add the scope to the context variable? Why is this meaningfully different from creating multiple keys prefixed with the extra string?
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.
See #26982 for my use case that prompted this.
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.
Why is this better than creating multiple context keys? Is it easier to track, easier to use, does it give a way to group related values together?
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.
Ah. If I understand your question correctly now, the upshot is that you otherwise can't! The context keys are determined by the construct (eg, when you call .fromLookup, it creates the key based on the lookup parameters), so this provides an escape hatch to customize them
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.
I'm not understanding the new capability you're proposing. Let's analyze an example. Today, if I want two values from lookup, I need two parameters, like this:
ssm.StringParameter.valueFromLookup(this, 'Parameter1');
ssm.StringParameter.valueFromLookup(this, 'Parameter2');
With your change, I have this now:
ssm.StringParameter.valueFromLookup(this, 'Parameter', 'extraKey1');
ssm.StringParameter.valueFromLookup(this, 'Parameter', 'extraKey2');
What does this 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.
This allows you to use the same parameter, but separate the cache lifetimes. The example I gave is with getting the latest Amazon Linux image - the name of the parameter is constant (AWS controls it!), and when spinning up new stateful instances I want the cache to include the value of the parameter at the time the instance was launched (which will be different for each instance).
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.
Ah! Now this is making sense, thank you for patiently explaining this. So when you pass the construct tree path (this.node.path
) as the extra cache key and synthesize, the result of the parameter lookup is stored under Parameter
as the top-level key, then /path/to/my/instance
as the cache key. As you add more instances, you're adding another entry under Parameter
to be path/to/my/instance2
.
These all hold the same value. When the parameter changes, those previous context entries continue to retain that value. When you create new instances after the parameter changes though, you get another entry under Parameter
, this time it's path/to/my/instance3
, but it has a different value than the first two.
This is a really cool feature! Please update the READMEs to explain some of these details about the cache lifetimes. Also please give an example walkthrough, where you detail the value of a parameter, then show the context after doing a lookup with a certain lifetime key, and then show what happens when the parameter changes and you perform another lookup. This will help users understand how this feature works.
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.
Strictly speaking the path and the parameter name are concatenated to get the cache key, but you get it 🙂
I agree I should work on expanding the documentation, though I need to figure out how to balance descriptiveness and conciseness (I got what I interpreted as conflicting feedback on this previously)
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.
Please lean on the side of overly descriptive initially and then we can trim it down later, as needed.
This PR has been in the MERGE CONFLICTS state for 3 weeks, and looks abandoned. To keep this PR from being closed, please continue work on it. If not, it will automatically be closed in a week. |
This PR has been deemed to be abandoned, and will be automatically closed. Please create a new PR for these changes if you think this decision has been made in error. |
Comments on closed issues and PRs are hard for our team to see. |
The ContextProvider mechanism and various "lookup" functions of a number of constructs support caching resolved values in the cdk.context.json. The context keys are constructed from the parameters of the lookup, which for lookup functions means whenever a resource with the same parameters is resolved, it is resolved as the same value across the entire app. However when a value may change over time, the user may wish to use the latest value when creating creating a new reference to the construct, effectively tying the cached context value to the scope - this patch enables this.
The primary use case is looking up an AMI parameter for a "stateful" EC2 instance. Currently if you specify cachedInContext, any future images created would use the same cached AMI, and updating the value would require updating all usages of the image across the entire app.
Closes #26982
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license