Skip to content

fixes IngressControllerCaptureHTTPCookie#776

Merged
openshift-merge-robot merged 2 commits intoopenshift:masterfrom
p0lyn0mial:fix-ingress-controller-capture-union
Nov 2, 2020
Merged

fixes IngressControllerCaptureHTTPCookie#776
openshift-merge-robot merged 2 commits intoopenshift:masterfrom
p0lyn0mial:fix-ingress-controller-capture-union

Conversation

@p0lyn0mial
Copy link
Contributor

as per https://github.com/kubernetes/enhancements/blob/master/keps/sig-api-machinery/20190325-unions.md#go-tags all fields inside a union must be optional.

this PR moves all optional fields to a new struct IngressControllerCaptureHTTPCookieUnion and leaves the required in the top level struct

as per https://github.com/kubernetes/enhancements/blob/master/keps/sig-api-machinery/20190325-unions.md#go-tags all fields inside a union must be optional.

this PR moves all optional fields to a new struct IngressControllerCaptureHTTPCookieUnion and leaves the required in the top level struct
@p0lyn0mial
Copy link
Contributor Author

/assign @sttts

@p0lyn0mial p0lyn0mial force-pushed the fix-ingress-controller-capture-union branch from 7647134 to 513dd66 Compare November 2, 2020 13:49
// captured.
// +union
type IngressControllerCaptureHTTPCookie struct {
IngressControllerCaptureHTTPCookieUnion `json:",inline"`
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

callers should start using IngressControllerCaptureHTTPCookieUnion instead

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

new union types should be directly embedded to IngressControllerCaptureHTTPCookie

@sttts
Copy link
Contributor

sttts commented Nov 2, 2020

/approve
/lgtm

/hold

to give @openshift/sig-network-edge some time to review

@openshift-ci-robot openshift-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Nov 2, 2020
@p0lyn0mial p0lyn0mial force-pushed the fix-ingress-controller-capture-union branch from 513dd66 to 64833fe Compare November 2, 2020 14:31
@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Nov 2, 2020
@Miciah
Copy link
Contributor

Miciah commented Nov 2, 2020

Thanks!
/hold cancel
/lgtm

@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 2, 2020
@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Nov 2, 2020
@openshift-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Miciah, p0lyn0mial, sttts

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-merge-robot openshift-merge-robot merged commit 9252afb into openshift:master Nov 2, 2020
candita added a commit to candita/cluster-ingress-operator that referenced this pull request Nov 18, 2020
… in AWS Route 53, also fix unit test issues introduced in openshift/api#776
candita added a commit to candita/cluster-ingress-operator that referenced this pull request Nov 18, 2020
… in AWS Route 53, also bump version and fix unit test issues introduced in openshift/api#776
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants