Skip to content
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.

Sign up for GitHub

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

Jump to bottom

chore(deps): bump to kube 1.17.3 #1370

Merged
merged 2 commits into from
Mar 24, 2020
Merged

chore(deps): bump to kube 1.17.3 #1370

merged 2 commits into from
Mar 24, 2020

Conversation

anik120
Copy link
Contributor

@anik120 anik120 commented Mar 13, 2020 •
edited
Loading

Description of the change:

  • Updates kube dependencies to 1.17.3.
  • Removes dependency on k8s.io/kubernetes my manually
    vendoring 1.17 version of it inside pkg/lib/ as a stop gap
    measure till other alternatives to rbac authorizer and any
    other parts of k8s.io/kubernetes used in olm is found.
  • Regenerate API types in accordance with openapi3
    specification

Reviewer Checklist

  • Implementation matches the proposed design, or proposal is updated to match implementation
  • Sufficient unit test coverage
  • Sufficient end-to-end test coverage
  • Docs updated or added to /docs
  • Commit messages sensible and descriptive

@anik120 anik120 mentioned this pull request Mar 13, 2020
Closed
5 tasks
@anik120
Copy link
Contributor Author

anik120 commented Mar 13, 2020

/hold

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 13, 2020
Bowenislandsong
Bowenislandsong requested changes Mar 13, 2020
View reviewed changes
Copy link
Member

@Bowenislandsong Bowenislandsong left a comment

Choose a reason for hiding this comment

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

Thanks for doing this. I have 2 questions:

  1. Why are we no longer using replace to pin kube versions like what openshift is doing?
  2. Would you consider using the solution here: 'unknown revision v0.0.0' errors, seemingly due to 'require k8s.io/foo v0.0.0' kubernetes/kubernetes#79384?

@anik120 anik120 force-pushed the bump-k8s-1.17-rm-k/k branch from cbe3ff4 to 7bb27aa Compare March 16, 2020 19:06
@anik120
Copy link
Contributor Author

anik120 commented Mar 16, 2020

/hold-cancel
@Bowenislandsong please refer operator-framework/operator-registry#201
Specifically,

Since k8s repos now offer a "native" way to pin versions k8s 1.16.7 maps to a go.mod version of v0.16.7, we can achieve the pinning to a particular kubernetes version without using the replace directive.

ecordell
ecordell reviewed Mar 16, 2020
View reviewed changes
go.mod Outdated
sigs.k8s.io/controller-tools v0.2.4
)

// NOTE: k8s.io/Kubernetes v1.17 is currently vendored by hand in pkg/lib and should be
// bumped along with other kube modules unitll dependency on k8s.io/Kubernetes is removed

replace (
github.com/docker/docker => github.com/moby/moby v0.7.3-0.20190826074503-38ab9da00309 // Required by Helm
github.com/openshift/api => github.com/openshift/api v3.9.1-0.20190924102528-32369d4db2ad+incompatible
Copy link
Member

Choose a reason for hiding this comment

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

Can this dependency be fixed as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ecordell #1370 (comment)

ecordell
ecordell reviewed Mar 16, 2020
View reviewed changes
go.mod
k8s.io/sample-apiserver => k8s.io/sample-apiserver v0.0.0-20190918161442-d4c9c65c82af
k8s.io/sample-cli-plugin => k8s.io/sample-cli-plugin v0.0.0-20190918162410-e45c26d066f2
k8s.io/sample-controller => k8s.io/sample-controller v0.0.0-20190918161628-92eb3cb7496c
sigs.k8s.io/structured-merge-diff => sigs.k8s.io/structured-merge-diff v1.0.1-0.20191108220359-b1b620dd3f06
Copy link
Member

Choose a reason for hiding this comment

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

this doesn't work if we just require 1.0.1?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ecordell unfortunately, this is the version that k8s.io/apiserver needs, but there is another module that is changing the version of sigs.k8s.io/structured-merge-diff to a version that is breaking for v0.17.3 of k8s.io/apiserver. So had to pin this version to appease all modules.

@anik120 anik120 force-pushed the bump-k8s-1.17-rm-k/k branch from 7bb27aa to f66fde1 Compare March 16, 2020 19:56
Bowenislandsong
Bowenislandsong requested changes Mar 16, 2020
View reviewed changes
Copy link
Member

@Bowenislandsong Bowenislandsong left a comment

Choose a reason for hiding this comment

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

Thanks for the update. We would want this changed to master or whatever openshift/oc is using as well.

go.mod Outdated
@@ -14,8 +15,8 @@ require (
github.com/mitchellh/hashstructure v1.0.0
github.com/openshift/api v3.9.1-0.20190924102528-32369d4db2ad+incompatible
Copy link
Member

Choose a reason for hiding this comment

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

you might wan to fix this!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Bowenislandsong thanks for noticing it! I've changed it to the tag being used by openshift/oc currently.

@anik120
Copy link
Contributor Author

anik120 commented Mar 16, 2020

/retest unit
/retest verify

@anik120 anik120 force-pushed the bump-k8s-1.17-rm-k/k branch from f66fde1 to b4a603f Compare March 16, 2020 20:47
@anik120
Copy link
Contributor Author

anik120 commented Mar 17, 2020

/hold cancel
/retest

@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 Mar 17, 2020
@anik120
Copy link
Contributor Author

anik120 commented Mar 17, 2020

/test unit

@anik120
Copy link
Contributor Author

anik120 commented Mar 17, 2020

/test e2e-gcp-upgrade

1 similar comment
@anik120
Copy link
Contributor Author

anik120 commented Mar 17, 2020

/test e2e-gcp-upgrade

@anik120
Copy link
Contributor Author

anik120 commented Mar 19, 2020 •
edited
Loading

Updated PR to include a commit that updates API types in accordance with updated openapi specifications.

References:
What are the annotations for in the commit?
How can I verify if the annotations are correct?
Why are the variable names being changed in the commit?

@LouisPlisso
Copy link

Thanks for doing this: hopefully soon the replace usage will be rare and we'll be able to import without problems :)

@anik120 anik120 force-pushed the bump-k8s-1.17-rm-k/k branch from fb47fbe to f8f75f6 Compare March 19, 2020 19:23
@anik120
Copy link
Contributor Author

anik120 commented Mar 19, 2020

/test e2e-gcp
/test e2e-aws-console-olm

1 similar comment
@anik120
Copy link
Contributor Author

anik120 commented Mar 19, 2020

/test e2e-gcp
/test e2e-aws-console-olm

@anik120
Copy link
Contributor Author

anik120 commented Mar 20, 2020

/retest

@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 20, 2020
anik120 added 2 commits March 20, 2020 15:33
@anik120
chore(deps): bump to kube 1.17.3
344f1a3 
* Updates kube dependencies to 1.17.3.
* Also removes dependency on k8s.io/kubernetes my manually
vendoring 1.17 version of it inside pkg/lib/ as a stop gap
measure till other alternatives to rbac authorizer and any
other parts of k8s.io/kubernetes used in olm is found.
@anik120
chore(apis): Regenerate API types for openapi3 specification
57b580e 
In kube 1.17, the openapi structural schema validation was updated to validate
CRDs according to the documented API semantics of x-kubernetes-list-type and
x-kubernetes-map-type atomic to reject non-atomic sub-types. This PR updates
the API types to follow the specification.
@anik120 anik120 force-pushed the bump-k8s-1.17-rm-k/k branch from f8f75f6 to 57b580e Compare March 21, 2020 03:05
@openshift-ci-robot openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 21, 2020
@openshift-bot
Copy link
Contributor

/retest

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

27 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-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-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-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-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-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-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-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-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-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-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-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-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-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 915c118 into operator-framework:master Mar 24, 2020
@ecordell ecordell mentioned this pull request Mar 24, 2020
Closed
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@awgreene awgreene awgreene requested changes

@ecordell ecordell ecordell left review comments

@Bowenislandsong Bowenislandsong Bowenislandsong requested changes

@exdx exdx Awaiting requested review from exdx

Assignees

@awgreene awgreene

@Bowenislandsong Bowenislandsong

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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@anik120 @LouisPlisso @Bowenislandsong @ecordell @awgreene @openshift-ci-robot @openshift-bot @openshift-merge-robot