Skip to content

Conversation

@runcom
Copy link
Member

@runcom runcom commented Apr 9, 2019

Signed-off-by: Antonio Murdaca [email protected]

- What I did

Bump openshift/api as per https://bugzilla.redhat.com/show_bug.cgi?id=1697657 and other k8s deps as needed by the openshift/api bump

- How to verify it

CI

- Description for the changelog

@openshift-ci-robot openshift-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Apr 9, 2019
@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 9, 2019
@eparis
Copy link
Member

eparis commented Apr 9, 2019

openshift/api#278

@rphillips
Copy link
Contributor

To fix the unit tests we need to bump client-go as well.

Depends on: openshift/client-go#100

@runcom
Copy link
Member Author

runcom commented Apr 9, 2019

To fix the unit tests we need to bump client-go as well.

Depends on: openshift/client-go#100

@rphillips so I should wait on that to merge?

@rphillips
Copy link
Contributor

@runcom yes, and we will need to regenerate the deps update on this PR after the client-go patch merges in.

@runcom
Copy link
Member Author

runcom commented Apr 9, 2019

To fix the unit tests we need to bump client-go as well.

actually, I don't think this is the case. The only units failing are related to the fact that the container_runtime controller reports the REAL error on failure, where the kubelet_config just silently ignores it see #603

In there, we talked about it, and right now, if something goes wrong the controller isn't throwing an error cause UpdateStatus succedes (again, the container runtime controller does the right thing by returning the actual error)

@rphillips
Copy link
Contributor

I'm seeing this error in the build log:

# github.com/openshift/machine-config-operator/vendor/github.com/openshift/client-go/config/clientset/versioned/typed/config/v1/fake
vendor/github.com/openshift/client-go/config/clientset/versioned/typed/config/v1/fake/fake_apiserver.go:110:48: too many arguments in call to testing.NewRootPatchSubresourceAction
	have (schema.GroupVersionResource, string, types.PatchType, []byte, []string...)
	want (schema.GroupVersionResource, string, []byte, ...string)
vendor/github.com/openshift/client-go/config/clientset/versioned/typed/config/v1/fake/fake_authentication.go:110:48: too many arguments in call to testing.NewRootPatchSubresourceAction
	have (schema.GroupVersionResource, string, types.PatchType, []byte, []string...)
	want (schema.GroupVersionResource, string, []byte, ...string)
vendor/github.com/openshift/client-go/config/clientset/versioned/typed/config/v1/fake/fake_build.go:99:48: too many arguments in call to testing.NewRootPatchSubresourceAction
	have (schema.GroupVersionResource, string, types.PatchType, []byte, []string...)
	want (schema.GroupVersionResource, string, []byte, ...string)
vendor/github.com/openshift/client-go/config/clientset/versioned/typed/config/v1/fake/fake_clusteroperator.go:110:48: too many arguments in call to testing.NewRootPatchSubresourceAction
	have (schema.GroupVersionResource, string, types.PatchType, []byte, []string...)
	want (schema.GroupVersionResource, string, []byte, ...string)
vendor/github.com/openshift/client-go/config/clientset/versioned/typed/config/v1/fake/fake_clusterversion.go:110:48: too many arguments in call to testing.NewRootPatchSubresourceAction
	have (schema.GroupVersionResource, string, types.PatchType, []byte, []string...)
	want (schema.GroupVersionResource, string, []byte, ...string)
vendor/github.com/openshift/client-go/config/clientset/versioned/typed/config/v1/fake/fake_console.go:110:48: too many arguments in call to testing.NewRootPatchSubresourceAction
	have (schema.GroupVersionResource, string, types.PatchType, []byte, []string...)
	want (schema.GroupVersionResource, string, []byte, ...string)
vendor/github.com/openshift/client-go/config/clientset/versioned/typed/config/v1/fake/fake_dns.go:110:48: too many arguments in call to testing.NewRootPatchSubresourceAction
	have (schema.GroupVersionResource, string, types.PatchType, []byte, []string...)
	want (schema.GroupVersionResource, string, []byte, ...string)
vendor/github.com/openshift/client-go/config/clientset/versioned/typed/config/v1/fake/fake_featuregate.go:110:48: too many arguments in call to testing.NewRootPatchSubresourceAction
	have (schema.GroupVersionResource, string, types.PatchType, []byte, []string...)
	want (schema.GroupVersionResource, string, []byte, ...string)
vendor/github.com/openshift/client-go/config/clientset/versioned/typed/config/v1/fake/fake_image.go:110:48: too many arguments in call to testing.NewRootPatchSubresourceAction
	have (schema.GroupVersionResource, string, types.PatchType, []byte, []string...)
	want (schema.GroupVersionResource, string, []byte, ...string)
vendor/github.com/openshift/client-go/config/clientset/versioned/typed/config/v1/fake/fake_infrastructure.go:110:48: too many arguments in call to testing.NewRootPatchSubresourceAction
	have (schema.GroupVersionResource, string, types.PatchType, []byte, []string...)
	want (schema.GroupVersionResource, string, []byte, ...string)

@runcom
Copy link
Member Author

runcom commented Apr 9, 2019

I'm seeing this error in the build log:

yeah, but that doesn't relate to openshift/client-go#100 - I mean that PR is probably still required but bumping client-go locally just worked for me and then there's the unit error I mentioned above related to kubelet_config. I'm creating an issue to be more precise.

@runcom
Copy link
Member Author

runcom commented Apr 9, 2019

/retest

@rphillips
Copy link
Contributor

Looks like we are dependent on a new k8s.io/client-go with this patch to bring in MergePatchType support.

@runcom
Copy link
Member Author

runcom commented Apr 9, 2019

Looks like we are dependent on a new k8s.io/client-go with this patch to bring in MergePatchType support.

oh nice, that would be perfect and fixes the units that are failing with container_runtime units.

Any eta on when that's gonna land in openshift/client-go?

@abhinavdahiya
Copy link
Contributor

looking at the diff in Gopkg.lock which is updating each package, but the commit message being bump openshift/api

you probably want to run dep ensure -update github.com/openshift/api github.com/openshift/client-go compared to dep ensure -update

@rphillips
Copy link
Contributor

@abhinavdahiya thanks for the tip, because I was trying a bunch of things with dep and failing.

diff --git a/Gopkg.toml b/Gopkg.toml
index ae550eab..d8cdd0e5 100644
--- a/Gopkg.toml
+++ b/Gopkg.toml
@@ -105,7 +105,7 @@ required = [
 
 [[constraint]]
   name = "k8s.io/api"
-  branch = "release-1.13"
+  branch = "release-1.14"
 
 [[constraint]]
   name = "k8s.io/apiextensions-apiserver"
@@ -113,7 +113,7 @@ required = [
 
 [[constraint]]
   name = "k8s.io/client-go"
-  version = "10.0.0"
+  version = "v11.0.0"
 
 [[constraint]]
   name = "k8s.io/code-generator"

[Note: v11.0.0 is not a typo, upstream added a 'v' in front of the version]

Then

dep ensure -v -update github.com/openshift/api github.com/openshift/client-go k8s.io/client-go

After doing these steps I got a successful make test.

@runcom runcom changed the title Bug 1697657: vendor: bump openshift/api Bug 1697657: vendor: bump openshift/api and needed k8s deps Apr 9, 2019
@runcom
Copy link
Member Author

runcom commented Apr 9, 2019

alright, I just -updateon openshift/api and openshift/client-go and the kube deps that we need to correctly generate the code for 1.13 (not really going to use 1.14 code).

For the failing units, I'm trying to find a nice hack while we move to 1.14 as the patch to backport just for that is huge and won't be backported afaict.

@runcom
Copy link
Member Author

runcom commented Apr 9, 2019

ok, last commit adds a workaround till we upgrade to 1.14 (not soon likely) @rphillips ptal I'll add something similar for kubelet controller once #617 merges and I rebase this

@runcom
Copy link
Member Author

runcom commented Apr 10, 2019

/retest

@runcom
Copy link
Member Author

runcom commented Apr 10, 2019

Ok, this should be good to go now 🎉

@rphillips
Copy link
Contributor

/lgtm

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@eparis
Copy link
Member

eparis commented Apr 12, 2019

no, you do not need to bump golang. it sohuld be fine.

@runcom
Copy link
Member Author

runcom commented Apr 12, 2019

no, you do not need to bump golang. it sohuld be fine.

alrighty, but I'll just leave it written here that we're bumping all our kube* deps to 1.13.4 then to bump openshift/api 😄

@runcom
Copy link
Member Author

runcom commented Apr 13, 2019

/retest

1 similar comment
@runcom
Copy link
Member Author

runcom commented Apr 13, 2019

/retest

@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Apr 13, 2019
@runcom
Copy link
Member Author

runcom commented Apr 13, 2019

New changes are detected. LGTM label has been removed.

I've rebased this and added a commit which aligns our kube deps to 1.13.x which is the one we support in 4.1

@kikisdeliveryservice @cgwalters @LorbusChris ptal again pls

@runcom
Copy link
Member Author

runcom commented Apr 14, 2019

/retest

runcom added 3 commits April 14, 2019 13:34
we need this workaround to mock out patch calls in unit until openshift#611 (comment)
which is probably going to be in kube 1.14

Signed-off-by: Antonio Murdaca <[email protected]>
Signed-off-by: Antonio Murdaca <[email protected]>
@runcom
Copy link
Member Author

runcom commented Apr 14, 2019

/retest

@runcom
Copy link
Member Author

runcom commented Apr 14, 2019

And finally green again

@cgwalters
Copy link
Member

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 15, 2019
@LorbusChris
Copy link
Contributor

/lgtm

@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cgwalters, kikisdeliveryservice, LorbusChris, rphillips, runcom

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:
  • OWNERS [LorbusChris,cgwalters,kikisdeliveryservice,runcom]

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

@runcom
Copy link
Member Author

runcom commented Apr 15, 2019

/retest

2 similar comments
@runcom
Copy link
Member Author

runcom commented Apr 15, 2019

/retest

@LorbusChris
Copy link
Contributor

/retest

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

2 similar comments
@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-merge-robot openshift-merge-robot merged commit 2e48ac5 into openshift:master Apr 16, 2019
@runcom runcom deleted the bump-oapi branch April 16, 2019 08:25
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. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

10 participants