-
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(ssm): allow disabling context caching in valueFromLookup #29372
Changes from all commits
babb36b
ed439fc
45b644b
f36f903
f7a7ae6
196f8d9
da5a439
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -482,6 +482,11 @@ | |
"props": { | ||
"$ref": "#/definitions/ContextQueryProperties", | ||
"description": "A set of provider-specific options." | ||
}, | ||
"disableContextCaching": { | ||
"description": "Whether to disable persisting this to the context file.", | ||
"default": false, | ||
"type": "boolean" | ||
Comment on lines
+485
to
+489
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Curious about the schema changes, did you add these manually? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not manual, no. If I remember correctly this was the result of running |
||
} | ||
}, | ||
"required": [ | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1 +1 @@ | ||
{"version":"36.0.0"} | ||
{"version":"37.0.0"} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -69,6 +69,11 @@ export async function provideContextValues( | |
}, resolvedEnvironment, sdk); | ||
|
||
value = await provider.getValue({ ...missingContext.props, lookupRoleArn: arns.lookupRoleArn }); | ||
|
||
if (missingContext.disableContextCaching) { | ||
debug(`Skipping context caching for ${key}`); | ||
context.set(key + TRANSIENT_CONTEXT_KEY, { [TRANSIENT_CONTEXT_KEY]: true }); | ||
Comment on lines
+73
to
+75
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does this property work for any context lookup? Like it was suggested in this comment There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It should be able to be used it in other context lookups, but this PR only implements it for SSM. I'm not sure what other context lookups would benefit from this, but with the underlaying changes in this PR, it would be easy to make it work elsewhere. |
||
} | ||
} catch (e: any) { | ||
// Set a specially formatted provider value which will be interpreted | ||
// as a lookup failure in the toolkit. | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -137,3 +137,18 @@ test('transient values arent saved to disk', async () => { | |
// THEN | ||
expect(config2.context.get('some_key')).toEqual(undefined); | ||
}); | ||
|
||
test('transient keys arent saved to disk', async () => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not sure about the purpose of this test since it doesn't use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a copy of the test above it ("transient values arent saved to disk") but it checks that the specified keys/values don't get saved to the context, regardless of what the value is. |
||
// GIVEN | ||
const config1 = await new Configuration({ readUserContext: false }).load(); | ||
config1.context.set('some_key', 'some_value'); | ||
config1.context.set('some_key' + TRANSIENT_CONTEXT_KEY, { [TRANSIENT_CONTEXT_KEY]: true }); | ||
await config1.saveContext(); | ||
expect(config1.context.get('some_key')).toEqual('some_value'); | ||
|
||
// WHEN | ||
const config2 = await new Configuration({ readUserContext: false }).load(); | ||
|
||
// THEN | ||
expect(config2.context.get('some_key')).toEqual(undefined); | ||
}); |
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.
Confused by why this is true, did you set value to
dummy-value-for-my-param-name
somewhere?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 here:
aws-cdk/packages/aws-cdk-lib/aws-ssm/README.md
Lines 61 to 65 in ab94070
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.
Also, if you look at the test before this one, a similar assertion is made (line 641)