-
Notifications
You must be signed in to change notification settings - Fork 13
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
feat(cli): can ignore errors and return dummy value in CloudControl API context provider #211
base: main
Are you sure you want to change the base?
Conversation
Head branch was pushed to by a user without write access
}); | ||
result.ResourceDescriptions?.forEach((resource) => { | ||
const id = resource.Identifier ?? ''; | ||
if (id !== '') { | ||
const propsObject = JSON.parse(resource.Properties ?? ''); | ||
|
||
const filters = Object.entries(propertyMatch); | ||
const filters = Object.entries(args.propertyMatch!); |
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.
Passing the individual fields of type string
, instead of the containing struct with fields of type string | undefined
, was done exactly so that we could centralize the "do the fields make sense" logic into one place and proceed under the certainty that we have types that make sense; so that we don't have to have sneaky !
operators in downstream code that are hard to miss.
If you really feel that it becomes too onerous to pass all fields of this struct, then define a local type alias with a tester, and pass that instead:
type CcApiContextQueryList = Required<Pick<CcApiContextQuery, 'Identifier' | 'typeName' | 'propertyMatch' | ... >>;
function isListQuery(x: CcApiContextQuery): x is CcApiContextQueryList {
return /* ... */;
}
const dummyValue = this.getDummyValueIfErrorIgnored(args); | ||
if (dummyValue) { | ||
const propsObj = getResultObj(dummyValue, 'dummy-id', args.propertiesToReturn); | ||
resultObjs.push(propsObj); | ||
return resultObjs; | ||
} |
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.
Should this behavior be in a try/catch
in getValue()
? It seems duplicated.
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: let's decide what a good error protocol would be here. I'm not convinced that return a single dummy object (which a caller has to supply a value for, and then has to test for that value... and it needs to be different from the default dummy object) is the best protocol.
Also; context provider return values are usually cached. Do we really want "not found" dummy values to be cached? (I.e., if they notice and create the resource after the first synth, we will still behave as if the resource doesn't exist).
Is that behavior going to match expected user behavior?
StorageEncrypted: 'true', | ||
}, | ||
], | ||
} as TestQueryWithDummy); |
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.
Use satisfies
instead of as
everywhere.
interface TestQueryWithDummy extends CcApiContextQuery { | ||
ignoreErrorOnMissingContext?: boolean; | ||
dummyValue?: any; | ||
} |
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 confused. Shouldn't these fields be part of CcApiContextQuery
itself?
Why do we define them only for the tests, and access them with introspection in getDummyValueIfErrorIgnored
?
CDK app with CC API Context Provider fails if the resource we want to get doesn't exist.
Also CC API Context Provider (
CcApiContextProviderPlugin
) cannot ignore errors even ifignoreErrorOnMissingContext
is passed in aws-cdk-lib because the current code in aws-cdk-cli doesn't handle the property.Sample cdk-lib code (based on my PR for IAM Role fromLookup):
However it would be good if the provider can ignore errors and return any dummy value to cdk library. This allows all resources to be used if available, or created if not.
Actually, SSM and KMS provider can ignore errors:
KMS: https://github.com/aws/aws-cdk-cli/blob/main/packages/aws-cdk/lib/context-providers/keys.ts#L43-L48
SSM: https://github.com/aws/aws-cdk-cli/blob/main/packages/aws-cdk/lib/context-providers/ssm-parameters.ts#L27-L30
For example, in cdk-lib, we can specify ignoreErrorOnMissingContext in the
fromLookup
. ThedummyValue
andignoreErrorOnMissingContext
properties can also be specified in GetContextValueOptions.cli-integ
aws/aws-cdk-cli-testing#50
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license