-
Notifications
You must be signed in to change notification settings - Fork 82
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
Ignore CEL expressions for CRDs #46
Conversation
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.
Thank you michael! I had few nits below~
internal/resourcegroup/builder.go
Outdated
// TODO(michaelhtm): CRDs are not supported for extraction currently | ||
// implement new logic specific to CRDs | ||
if gvk.Kind == "CustomResourceDefinition" { | ||
emulatedResource, err := parser.ParseSchemalessResource(unstructuredResource) |
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.
emulatedResource, err := parser.ParseSchemalessResource(unstructuredResource) | |
celExpressions, err := parser.ParseSchemalessResource(unstructuredResource) |
internal/resourcegroup/builder.go
Outdated
if emulatedResource != nil { | ||
return nil, fmt.Errorf("failed, CEL expressions are not supported for CRDs, resource %s", rgResource.Name) | ||
} |
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.
slices can be non-nil and contain 0 elements
if emulatedResource != nil { | |
return nil, fmt.Errorf("failed, CEL expressions are not supported for CRDs, resource %s", rgResource.Name) | |
} | |
if len(emulatedResource) > 0 { | |
return nil, fmt.Errorf("failed, CEL expressions are not supported for CRDs, resource %s", rgResource.Name) | |
} |
internal/resourcegroup/builder.go
Outdated
if err != nil { | ||
return nil, fmt.Errorf("failed to extract CEL expressions from schema for resource %s: %w", rgResource.Name, err) | ||
} | ||
resourceVariables = []*ResourceVariable{} |
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.
resourceVariable
is already redeclared in L125
internal/resourcegroup/builder.go
Outdated
}) | ||
// TODO(michaelhtm): CRDs are not supported for extraction currently | ||
// implement new logic specific to CRDs | ||
if gvk.Kind == "CustomResourceDefinition" { |
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 we check for the full GVK instead of just the Kind name?
c9fbffe
to
5bc154b
Compare
We are deciding to ignore CEL expressions for CRDs for now, since CRDs get some variables injected by kubernetes that don't have specific types.
5bc154b
to
a9bfad2
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.
First code contribution to symphony :) 🚀
/lgtm
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: a-hilaly, michaelhtm The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Ignore CEL expressions for CRDs
Description of changes:
We are deciding to ignore CEL expressions for CRDs
for now, since CRDs get some variables injected
by kubernetes that don't have specific types.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.