-
Notifications
You must be signed in to change notification settings - Fork 414
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
Uses the new parsing structure for RBAC parsing #3206
base: main
Are you sure you want to change the base?
Conversation
Is there anything I can do to help you with this? |
…or into improve-rbac-parsing
internal/components/component.go
Outdated
type Option[T any] struct { | ||
protocol corev1.Protocol | ||
appProtocol *string | ||
targetPort intstr.IntOrString | ||
nodePort int32 | ||
name string | ||
port int32 | ||
portParser PortParser[T] | ||
rbacGen RBACRuleGenerator[T] | ||
} |
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.
Note: I need the generics for the parsing stuff, but it really crowds the type signatures. (see the receivers/helpers.go file) I could remove the functions from the options struct, and put it in the constructor, but that feels counter intuitive to the options struct.
internal/components/component.go
Outdated
@@ -75,22 +83,32 @@ func (o *Option) GetServicePort() *corev1.ServicePort { | |||
} | |||
} | |||
|
|||
type PortBuilderOption func(*Option) | |||
func WithRBACRuleGenerator[T any](r RBACRuleGenerator[T]) PortBuilderOption[T] { |
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.
TBH I don't feel confortable with everything returing "PortBuilder". For some components we don't have to build anything port related (some processors) but we need to get RBAC. I was not sure about this in the other PR but I though you were thinking about something different and didn't raise my concerns.
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.
I think i should just rename this to not be "PortBuilderOption" but rather "ParserOption" which is more accurate.
type PortRetriever interface { | ||
GetPortNum() (int32, error) | ||
GetPortNumOrDefault(logr.Logger, int32) int32 | ||
} | ||
|
||
type Option struct { |
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.
moved this to builder.go
if err := mapstructure.Decode(config, &parsed); err != nil { | ||
return nil, err | ||
} |
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.
By using generics here, we can let mapstructure decode take care of all the marshalling for us into the exact right type – no more map assertions!
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.
Great work! Thanks for taking care of this. When do we plan to start using the new infra and remove the old one?
this changes to use it for RBAC parsing, hoping to delete the old stuff in a different PR. |
…nto improve-rbac-parsing
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.
I'm not that familiar with the component parsing code, so I mostly limited my review to naming and whether the abstractions make sense to me in a vacuum. The test code looks good to be at a glance, but I'd feel more assured if someone with more expertise reviewed it before merging.
Description: Uses the new parsing structure for getting parsing rules
Link to tracking Issue(s):
Testing: Many unit tests
Documentation: n/a