Add top-level known referencers in single config#142
Add top-level known referencers in single config#142muvaf merged 4 commits intocrossplane-contrib:mainfrom
Conversation
Signed-off-by: Muvaffak Onus <[email protected]>
Signed-off-by: Muvaffak Onus <[email protected]>
Signed-off-by: Muvaffak Onus <[email protected]>
|
|
||
| // KnownReferencers adds referencers for fields that are known and common among | ||
| // more than a few resources. | ||
| func KnownReferencers() tjconfig.ResourceOption { //nolint:gocyclo |
There was a problem hiding this comment.
Have you considered the v1alpha2 bumps in this PR?
There was a problem hiding this comment.
I just rebased and fixed the paths to include v1alpha2
| switch { | ||
| case strings.HasSuffix(k, "role_arn"): | ||
| r.References[k] = tjconfig.Reference{ | ||
| Type: "github.com/crossplane-contrib/provider-jet-aws/apis/iam/v1alpha1.Role", |
There was a problem hiding this comment.
If these type constants are used elsewhere (for example in other custom configurations), you may consider defining constants for them.
There was a problem hiding this comment.
That's a good idea but I'd like to do that for manual referencers as well altogether in another PR.
| case strings.HasSuffix(k, "security_group_ids"): | ||
| r.References[k] = tjconfig.Reference{ | ||
| Type: "github.com/crossplane-contrib/provider-jet-aws/apis/ec2/v1alpha1.SecurityGroup", | ||
| RefFieldName: strings.TrimSuffix(name.NewFromSnake(k).Camel, "s") + "Refs", |
There was a problem hiding this comment.
What about caching strings.TrimSuffix(name.NewFromSnake(k).Camel, "s") in a variable?
There was a problem hiding this comment.
I think it reads better to have it contained like this and there isn't much gain/loss in performance.
| switch k { | ||
| case "vpc_id": | ||
| r.References["vpc_id"] = tjconfig.Reference{ | ||
| Type: "github.com/crossplane-contrib/provider-jet-aws/apis/ec2/v1alpha2.VPC", |
There was a problem hiding this comment.
Can we handle resolver generation if the referrer and the referee are in the same package?
There was a problem hiding this comment.
angryjet takes care of the case where the type is in the same package even if a full path is given. Though I'm not really sure what's going on here in these two. It prints an error talking about import cycle with autoscaling and for some reason adds an import to self. But I didn't have the patience to dig too deep.
| Type: "github.com/crossplane-contrib/provider-jet-aws/apis/ec2/v1alpha2.SecurityGroup", | ||
| } | ||
| case "kms_key_id": | ||
| if !r.TerraformResource.Schema["kms_key_id"].Sensitive { |
There was a problem hiding this comment.
Do we assume if these fields are sensitive, then they should be references to references to secrets (actual data, e.g., credentials) and not to another AWS object? Maybe a little comment would help.
There was a problem hiding this comment.
Just moved the sensitive check to top of the for loop. The reason we do this is that we use a secret ref for all fields that are sensitive and adding reference to them breaks the resolver since it's not a valid case. I'll add a comment.
Signed-off-by: Muvaffak Onus <[email protected]>
2 participants
Description of your changes
With this change, we get the following number of referencer configs on top of the existing ones:
Fixes #135
I have:
make reviewable testto ensure this PR is ready for review.How has this code been tested
make reviewable