-
Notifications
You must be signed in to change notification settings - Fork 453
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
Refactoring path modifier to use union discriminator #1124
Conversation
This is a relatively large change, adding a hold until we can get some broader consensus. /hold |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: robscott 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 |
a58e11a
to
a274447
Compare
There's a kubernetes-api-reviewers thread that's relevant to this PR. |
@@ -705,17 +705,25 @@ type HTTPPathModifier struct { | |||
// Type defines the type of path modifier. | |||
// | |||
// <gateway:experimental> | |||
// +kubebuilder:validation:Enum=Absolute;ReplacePrefixMatch | |||
// +kubebuilder:validation:Enum=ReplaceFullPath;ReplacePrefixMatch |
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.
Since you really don't have any other fields than union members in that struct, I don't think the discriminator is useful at all.
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 was me, I posted with a temporary account ;-)
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 this adds room for growth. The goal here is to add additional types in the future. Since we already have 14 implementations and integrations, and have more on the way, it's very likely that at least some of these will trail the version of the API deployed in the cluster. A discriminator field allows those implementations to communicate that they don't support a specific type, instead of just seeing an empty struct and not knowing what to do with it.
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.
Yeah, they don't get a lot more value from seeing the struct empty vs seeing the discrimator set to a value that doesn't make sense (but they get a little value). In both cases, they can manually clear everything/set the discriminator to the right value.
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 chatted with @thockin about this, and while I think he still has significant reservations about using a union discriminator, I think he's on board with starting with one. The idea is that it's impossible to retroactively add a discriminator, but we could always make a discriminator optional and/or provide a smart default in the future. (Please correct me if I misunderstood anything).
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.
LGTM with one caveat about the kubebuilder enum.
Discussed at community meeting today. Seems like there's consensus with this approach. I've pushed an update to address feedback and going to go ahead and remove the hold so we can get this in with the next LGTM. /hold cancel |
/lgtm |
What type of PR is this?
/kind cleanup
/kind api-change
What this PR does / why we need it:
This is a follow up to @thockin's feedback in #1086 (comment). It makes the following changes:
Does this PR introduce a user-facing change?:
Not sure this needs a release note because none of the updated fields were ever released, so up to author of changelog as far as if this should be included: