Align naming and return types for accessors in config package #327
Conversation
pkg/config/resource.go
Outdated
| func (c *Config) GetCompareIgnoredFields(resName string) []string { | ||
| // GetCompareIgnoredFields returns the list of field paths to ignore when | ||
| // comparing two different objects | ||
| func (c *Config) GetCompareIgnoredFields(resourceName string) []string { |
There was a problem hiding this comment.
We might want to rename this to GetCompareIgnoredFieldPaths to more accurately describe the function's returned value.
There was a problem hiding this comment.
@brycahta if you update this little guy, I'll merge away.
| } else if foundFieldRename { | ||
| msg := fmt.Sprintf( | ||
| "Field rename %s for operation %s is not part of %s Spec or"+ | ||
| " Status fields", memberName, op.Name, r.Names.Camel) | ||
| panic(msg) |
There was a problem hiding this comment.
I think this was an important panic() in the original code, no? It was informing the author of the generator.yaml file that they had specified in the generator.yaml file a field to be renamed that isn't in the Status or Spec...
There was a problem hiding this comment.
My thinking when removing:
- code generate package shouldn't need to worry or care if a field was renamed. just hand it over fields and generate Go code
- if we are unable to satisfy a rename request (i.e. unable to insert renamed field into Spec or Status), then we should panic when creating the CRD
Related to 2, was there a reason to not panic here (and similar for Status field)? Due to continue, I think this is the only possible way to trigger the panic() in the downstream, removed clause.
There was a problem hiding this comment.
code generate package shouldn't need to worry or care if a field was renamed. just hand it over fields and generate Go code
The problem with not panicking here is that we might experience different kinds of errors or even nil pointer panics in other parts of the code (which is kinda hard to debug, when it comes to the templating part). Forcing panics in similar areas helps the user understand the errors and fix them quickly.
Related to 2, was there a reason to not panic here (and similar for Status field)?
There are a lot of missing panics in the code generator. I personally believe we should add more of them to guide the user on passing valid configurations to the users.
There was a problem hiding this comment.
Bryan, you make a good point about how the panic() in the generation code paths improperly couples the inference piece with the generation piece. I think we can look at adding back some checks/panics in the inference part of the refactored code generator in the future and keep it nicely decoupled.
pkg/config/resource.go
Outdated
| func (c *Config) GetCompareIgnoredFields(resName string) []string { | ||
| // GetCompareIgnoredFields returns the list of field paths to ignore when | ||
| // comparing two different objects | ||
| func (c *Config) GetCompareIgnoredFields(resourceName string) []string { |
There was a problem hiding this comment.
@brycahta if you update this little guy, I'll merge away.
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: A-Hilaly, brycahta, jaypipes, vijtrip2 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 |
Issue #, if available: aws-controllers-k8s/community#1184
Description of changes:
Getand removeerrorandboolreturn typesBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.