Bug 1832153: Upstream v1.18.2 rebase #136
Conversation
openshift-ci-robot
added
the
do-not-merge/work-in-progress
openshift-ci-robot
requested review from
alexanderConstantinescu and
rcarrillocruz
openshift-ci-robot
added
the
approved
|
This currently points to a personal fork of kubernetes - once CI looks reasonable will push to origin. |
|
Oh, I hope you were aware of this this: #117 or at least that you didn't miss it |
Yup, I followed it - it worked quite well! |
| @@ -224,3 +225,19 @@ func GetHostIPNetworks(skipInterfaces []string) ([]*net.IPNet, []net.IP, error) | |||
| } | |||
| return hostIPNets, hostIPs, kerrors.NewAggregate(errList) | |||
| } | |||
|
|
|||
| func HSEgressIPsToStrings(ips []networkv1.HostSubnetEgressIP) []string { | |||
There was a problem hiding this comment.
Sigh. This is a mess and we should revert the openshift/api change that required it.
(At a minimum, split the egressip changes into a separate commit so they're easily revertable when we fix this later.)
There was a problem hiding this comment.
In case someone else gets here later and also wonders the rationale for this:
openshift/api@a63a88e
There was a problem hiding this comment.
grump: go has exactly two "magic" types. Why it can't implement type conversion across literally the only two polymorphic types in the whole world is infuriating. It can't even use the "oh, we actually monomorphize everything" excuse like Rust.
There was a problem hiding this comment.
@abhat can fill us in, but I believe the issue is that it's not possible to add validation to an array
There was a problem hiding this comment.
Yes, but we don't need validation there if it's just going to make our lives more difficult. We have to validate the network CRD types at runtime anyway because the CRD validation only handles part of it. There is talk about adding a ValidatingWebHook at some point so we can do this properly.
|
Looks good to me so far. Also checked your local 1.18 branch and only has the two old carry patches so it looks fine there too. I'm launching the e2e-gcp for you because I'm pretty sure the failing test is unrelated. |
|
okay, just waiting for someone with more power to push to |
|
ok, @danwinship I split the commits. Since CI passed, I've pushed the tag to openshift/kubernetes, and updated go.mod accordingly. This is ready to go. |
squeed
changed the title
openshift-ci-robot
added
bugzilla/severity-unspecified
|
@squeed: This pull request references Bugzilla bug 1832153, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker. 3 validation(s) were run on this bug
In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
openshift-ci-robot
added
the
bugzilla/valid-bug
|
Oof, looks like the verify-deps strikes again... @squeed the make update-deps-overrides produces different vendor depending on your go version, env, and god knows what. I strongly recommend that you get pull verify-deps pod image, run it locally, run the make update-deps-overrides inside that container and copy the contents of it into your vendor directory with docker cp or whatever container runtime engine you're using. |
Derp, forgot to |
Also, minor type renaming.
What were formerly strings are now named types.
- all client-go operations need a context - small test util renames
|
Dan observed that kubernetes branch should have been a fork off of upstream, not origin. Fixed that, revendored, repushed. |
|
lgtm but should we wait for openshift/kubernetes#126 first to avoid a second bump? |
Hah, I figured you'd be mad if I conflated that pr. |
|
/retest |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: danwinship, squeed 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 |
|
@squeed: All pull requests linked via external trackers have merged: openshift/sdn#136. Bugzilla bug 1832153 has been moved to the MODIFIED state. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
Nothing too exciting, but a lot of mechanical changes:
contextstringtypes now have explicit names, so lots of boilerplate