fix(core): account for { Ref } incompatibility between schema and CFN
#36493
Conversation
This is the change in CDK that consumes the spec update cdklabs/awscdk-service-spec#2328. This accounts for `{ Ref }` not always returning what the schema says that the primary identifier of a resource is. This is not unexpected or wrong: the Registry Schema describes the behavior of Cloud Control API, and the identifier uniquely identifies a resource in an account. We do need to account for the differences though. What we will do is: - Retain the CC-API fields in the reference interface if we hvae the values. - Fall back to the (smaller) CFN identifier fields if we don't have the values. - Make the CFN identifier fields win if the identifiers are equal size (in which case we probably a Name vs ARN distinction).
rix0rrr
added
the
pr-linter/exempt-integ-test
aws-cdk-automation
dismissed
their stale review
✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.
rix0rrr
force-pushed
the
huijbers/cfn-identifiers
branch
from
2134694 to
ddfb1a0
Compare
aws-cdk-automation
added
the
pr/needs-maintainer-review
| export function findNonIdentifierArnProperty(resource: Resource) { | ||
| return findArnProperty(resource, (name) => !resource.primaryIdentifier?.includes(name)); | ||
| const refIdentifier = resource.cfnRefIdentifier ?? resource.primaryIdentifier; | ||
| return findArnProperty(resource, (name) => !refIdentifier?.includes(name)); |
There was a problem hiding this comment.
i know you didn't add in the !, but if we're confident primaryIdentifier always exists, then why is the property typed as optional in the first place?
There was a problem hiding this comment.
I think we're confident that it exists here, not that it exists in general?
The property is probably optional because we added it later.
| * the `Arn`. We will just use the CFN value as leading. | ||
| * | ||
| * We will identify the difference between these 2 cases by the length of the primary | ||
| * identifier: equal length = aliasing, different length = specificity. |
There was a problem hiding this comment.
by length you mean length of the identifier array and not length of their string value, correct? i was slightly confused here at how we are identifying the difference between the two cases. this also means that we are certain that there is no case where the CC-API identifier is simply different than the CFN identifier (not aliased). we are certain this is the case?
also, are we using 'unique identifier' and 'primary identifier' interchangably?
There was a problem hiding this comment.
length of the identifier array and not length of their string value
Yes.
CC-API identifier is simply different than the CFN identifier (not aliased)
Well, "aliased" is probably a wrong name here but I couldn't think of a better one. I'm thinking of [Name] <-> [Arn]. That is indeed "different", but it's a different kind of different from [StageName] <-> [ApiId, StageName].
also, are we using 'unique identifier' and 'primary identifier' interchangably?
I guess so. I'm trying to put the distinction on CCAPI identifier vs CFN identifier, the other words are not different on purpose.
| // actual = actual.split('\n').map(x => x.trim()).join('\n'); | ||
| // subset = subset.split('\n').map(x => x.trim()).join('\n'); | ||
| // console.log(actual); |
Removed commented-out code for trimming and logging.
aws-cdk-automation
removed
the
pr/needs-maintainer-review
|
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
Merge Queue Status✅ The pull request has been merged at a071b05 This pull request spent 29 minutes 53 seconds in the queue, including 29 minutes 42 seconds running CI. Required conditions to merge
|
|
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
|
Comments on closed issues and PRs are hard for our team to see. |
3 participants
This is the change in CDK that consumes the spec update cdklabs/awscdk-service-spec#2328.
This accounts for
{ Ref }not always returning what the schema says that the primary identifier of a resource is. This is not unexpected or wrong: the Registry Schema describes the behavior of Cloud Control API, and the identifier uniquely identifies a resource in an account. We do need to account for the differences though.What we will do is:
This brings in a new version of service-spec so it also adds a new service.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license