rename TargetRefWithSectionName, add PolicyTargetReference #536
Conversation
|
Issues linked to changelog: |
| // * https://github.com/kubernetes-sigs/gateway-api/blob/444631bfe06f3bcca5d0eadf1857eac1d369421d/apis/v1alpha2/policy_types.go#L58-L83 | ||
| message TargetRefWithSectionName { | ||
| // * https://github.com/kubernetes-sigs/gateway-api/blob/b4f0307cc9269e73187300e72979e7e111ab74ab/apis/v1alpha2/policy_types.go#L58-L83 | ||
| message PolicyTargetReferenceWithSectionName { | ||
| string group = 1; |
There was a problem hiding this comment.
should the fields in here be updated to mirror what's in https://gateway-api.sigs.k8s.io/reference/spec/#gateway.networking.k8s.io%2fv1alpha2.PolicyTargetReferenceWithSectionName?
There was a problem hiding this comment.
Yeah, can the PolicyTargetReference be inlined here like upstream is doing? https://github.com/kubernetes-sigs/gateway-api/blob/b4f0307cc9269e73187300e72979e7e111ab74ab/apis/v1alpha2/policy_types.go#L68
There was a problem hiding this comment.
you can not have anonymous nested objects in proto (as opposed to go struct)
have you seen a way to accomplish this?
| google.protobuf.StringValue namespace = 4; | ||
| } | ||
|
|
||
| // PolicyTargetReferenceWithSectionName identifies Gateway API objects, and optionally a specific section of those objects, to directly apply policy to. | ||
| // This is a copy of the upstream K8s Gateway API `targetRef` API. |
There was a problem hiding this comment.
this comment is out of date, should be PolicyTargetReferenceWithSectionName API?
There was a problem hiding this comment.
the intent was the call out the generic targetRef concept not the specific message it is using.
I can update this though
| @@ -162,14 +162,28 @@ message ObjectSelector { | |||
| } | |||
| } | |||
|
|
|||
| // TargetRef identifies Gateway API objects to apply a direct policy to. | |||
| // PolicyTargetReference identifies Gateway API objects to directly apply policy to. | |||
| // This is a copy of the upstream K8s Gateway API `targetRef` API. | |||
There was a problem hiding this comment.
this comment is out of date, should be PolicyTargetReference API?
There was a problem hiding this comment.
Yeah, I think the link is out of date, is it this one? https://github.com/kubernetes-sigs/gateway-api/blob/b4f0307cc9269e73187300e72979e7e111ab74ab/apis/v1alpha2/policy_types.go#L39
There was a problem hiding this comment.
the intent was the call out the generic targetRef concept not the specific message it is using. will update though
There was a problem hiding this comment.
@npolshakova that link is the same as the one in the comment?
https://github.com/kubernetes-sigs/gateway-api/blob/b4f0307cc9269e73187300e72979e7e111ab74ab/apis/v1alpha2/policy_types.go#L34-L56
There was a problem hiding this comment.
That link works- I must have miscopied it when trying to find it!
| @@ -0,0 +1,4 @@ | |||
| changelog: | |||
There was a problem hiding this comment.
nit: since this is breaking, you could move to v0.37.0 and use type BREAKING_CHANGE
There was a problem hiding this comment.
the goal was to fix this before it became a breaking change as no one has consumed this message yet (see PR description for references to edge & GME)
the versioning is a bit flimsy in this repo (e.g. there is currently not a v0.36.x brach) so it seemed easiest to just break it now while there are no consumers.
that being said, it is technically a break from a semver perspective, so I'm not sure the best approach
renaming message field but not used in the most recent LTS releases:
https://github.com/solo-io/gloo-mesh-enterprise/blob/v2.5.x/go.mod#L95
https://github.com/solo-io/gloo/blob/v1.16.x/go.mod#L56
BOT NOTES:
resolves #529