-
Notifications
You must be signed in to change notification settings - Fork 17
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
Extend metadata with descriptors of CF Ref handling #1836
Conversation
Does the PR have any schema changes?Looking good! No breaking changes found. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1836 +/- ##
==========================================
- Coverage 49.96% 47.13% -2.84%
==========================================
Files 43 50 +7
Lines 6636 7050 +414
==========================================
+ Hits 3316 3323 +7
- Misses 3077 3483 +406
- Partials 243 244 +1 ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
7b66ded
to
f91f587
Compare
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.
Looks good! Most of my comments are just around documentation. This may be something that should live in pulumi-cdk, but we can always move it later.
) | ||
|
||
// Instead of humans playing the game, let the computer assign labels based on heuristics that seem to be fairly robust. | ||
// This should add a label in the data indicating that it was assigned by a heuristic, in case this data point ends up |
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.
👍🏻
continue | ||
} | ||
|
||
switch { |
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.
Can you add a comment to each one of these cases with a simple example? I think it would help in quickly understanding each one.
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.
It's a bit ad-hoc but I've added a quick comment.
var categorizationRules = []categorizationRule{ | ||
{ | ||
Category: RefReturnsArn, | ||
Pattern: regexp.MustCompile(`^The Amazon Resource Name ..ARN.. of the certificate authority.?.?$`), |
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.
Just curious, did you create these by hand? Do they map more than one resource? I'm curious how scalable this is.
Also can you add a comment explaining the above so we know the process for updating 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.
This is not supposed to be scalable, this is throw-away data cleaning code. I'll add a quick README. Once we complete curating a clean data set in ref-db.json we can delete this.
provider/tools/ref-parser/cat.go
Outdated
Pattern: regexp.MustCompile("When you pass the logical ID of this resource to the intrinsic .Ref.?.?function, .Ref.?.?returns a .* ID"), | ||
}, | ||
// { | ||
// Name: "RefReturnsArn", |
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.
Reminder.
|
||
What Ref returns varies from resource to resource. The only available information on this is free-form text in CloudFormation documentation and anything that can be extracted experimentally. To facilitate building tooling such as `pulumi/pulumi-cdk` that emulates CF behavior, `ref-db.json` collects schematized information describing the behavior of `Ref`. | ||
|
||
`ref-db.json` can be edited directly to correct errors or add entries for resources not yet covered. |
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.
Can you add an example here on what it looks like to update 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.
You send a PR and edit it? I'm not sure I understand. It just has to conform to the schema.
CloudFormation Ref intrinsic behavior varies from resource to resource. This change is an attempt to schematize behavior and expose is in the provider metadata. One intended use case is
pulumi/pulumi-cdk
CF emulator.This change implies the
ref-db.json
becomes the source of truth for this metadata and we maintain it as well as possible.About 1/2 of CF resources are currently covered in the file, including 81 of the top used resources (pulumi/pulumi-cdk#237). Data cleaning and data label attachment helper code is included but is not intended to be used in production but only to maintain
ref-db.json
.