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

Bump client-go and other deps to v1.18.3 #851

Merged
merged 4 commits into from Jul 22, 2020
Merged

Bump client-go and other deps to v1.18.3 #851

merged 4 commits into from Jul 22, 2020

Conversation

paulfantom
Copy link
Contributor

@paulfantom paulfantom commented Jul 15, 2020 •
edited

  • Updated client-go to 0.18.3

  • Updated prometheus-operator to 0.40.0 (necessary for client-go upgrade)

  • Updated other k8s deps to 0.18.X (required by prometheus-operator upgrade)

  • Updated prometheus to 2.19.0 (required by prometheus-operator upgrade)

  • Moving VolumeClaimTemplate type to EmbeddedPersistentVolumeClaim from prometheus-operator to fix type incompatibility

  • Removed setting CreationTimestamp to 0 as it is not required after changing types.

  • I added CHANGELOG entry for this change.
  • No user facing changes, so no entry in CHANGELOG was needed.

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 15, 2020
@paulfantom paulfantom changed the title Bump client-go and other deps to v1.18.3 WIP: Bump client-go and other deps to v1.18.3 Jul 15, 2020
@openshift-ci-robot openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 15, 2020
@paulfantom paulfantom force-pushed the client-go branch 2 times, most recently from c759cae to e9c478b Compare July 15, 2020 13:11
@paulfantom
Copy link
Contributor Author

/retest

@paulfantom paulfantom changed the title WIP: Bump client-go and other deps to v1.18.3 Bump client-go and other deps to v1.18.3 Jul 16, 2020
@openshift-ci-robot openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 16, 2020
@lilic
Copy link
Contributor

lilic commented Jul 16, 2020

Updated prometheus-operator to 0.40.0 (necessary for client-go upgrade)

I don't think there is a reason we should wait until 0.40 prom-operator bump as well right?

Is this ready for review now?

@paulfantom
Copy link
Contributor Author

I don't think there is a reason we should wait until 0.40 prom-operator bump as well right?

I don't think there is any.

Is this ready for review now?

yes

simonpasquier
simonpasquier reviewed Jul 16, 2020
View reviewed changes
go.mod
Comment on lines +36 to +33
k8s.io/api => k8s.io/api v0.18.3
k8s.io/apimachinery => k8s.io/apimachinery v0.18.3
k8s.io/client-go => k8s.io/client-go v0.18.3
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we really need these replacements?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Those are ensuring we are pinning to 0.18.3, without them go mod tidy rewrites it to v0.18.4 and in case of client-go it panics.

Copy link
Contributor

Choose a reason for hiding this comment

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

👍

go.mod Outdated
k8s.io/api => k8s.io/api v0.17.1
k8s.io/apimachinery => k8s.io/apimachinery v0.17.1
k8s.io/client-go => k8s.io/client-go v0.17.1
github.com/prometheus/prometheus => github.com/prometheus/prometheus v1.8.2-0.20200609102542-5d7e3e970602 // v1.8.2 is misleading as Prometheus does not have v2 module. This is pointing to v2.19.0, the same as in promehteus-operator v0.40.0
Copy link
Contributor

Choose a reason for hiding this comment

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

I suspect that you can get rid of the replacement and put it directly in the require list.

pkg/client/client.go Outdated
@@ -148,32 +149,33 @@ func (c *Client) SecretListWatchForNamespace(ns string) *cache.ListWatch {

func (c *Client) WaitForPrometheusOperatorCRDsReady() error {
return wait.Poll(time.Second, time.Minute*5, func() (bool, error) {
err := c.WaitForCRDReady(k8sutil.NewCustomResourceDefinition(monv1.DefaultCrdKinds.Prometheus, mon.GroupName, map[string]string{}, false))
// TODO(paulfantom): re-add checking for CRDs
Copy link
Contributor

Choose a reason for hiding this comment

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

to be removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed waiting for CRDs as those are managed externally.

lilic
lilic reviewed Jul 17, 2020
View reviewed changes
pkg/client/client.go Outdated Show resolved Hide resolved
pkg/client/client.go Outdated Show resolved Hide resolved
@simonpasquier simonpasquier mentioned this pull request Jul 17, 2020
Closed
14 tasks
@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 18, 2020
@openshift-ci-robot openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 20, 2020
@paulfantom
Copy link
Contributor Author

/refresh
/test generate

@paulfantom paulfantom closed this Jul 20, 2020
@paulfantom paulfantom reopened this Jul 20, 2020
@paulfantom paulfantom mentioned this pull request Jul 20, 2020
Closed
2 tasks
@paulfantom
Copy link
Contributor Author

This is now rebased on top of #870 to allow faster testing.

@paulfantom paulfantom force-pushed the client-go branch 2 times, most recently from 9fc9508 to e2dcff3 Compare July 20, 2020 14:32
lilic
lilic reviewed Jul 20, 2020
View reviewed changes
Copy link
Contributor

@lilic lilic left a comment

Choose a reason for hiding this comment

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

One question otherwise :shipit:

go.mod
@@ -4,34 +4,33 @@ go 1.13

Copy link
Contributor

Choose a reason for hiding this comment

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

Should we do go 1.14 here above?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think we have any features in CMO which need 1.14 and codebase can be compiled with 1.13, so the current setting seems good to me.

@paulfantom
pkg,test: bump dependencies to current versions and fix types
f4b5b9b 
* all k8s deps to 1.18.X
* prometheus-operator to 0.40.0
* prometheus to v2.19.0 (same as in promehteus-operator)
@paulfantom
vendor: revendor
23b3bd1
@lilic
Copy link
Contributor

lilic commented Jul 21, 2020

level=error msg="Error: Error launching source instance: InvalidParameterValue: Value (ci-op-r7mt5ftx-f8b9e-jr867-bootstrap-profile) for parameter iamInstanceProfile.name is invalid. Invalid IAM Instance Profile name"

aws failures
/retest

lilic
lilic approved these changes Jul 21, 2020
View reviewed changes
Copy link
Contributor

@lilic lilic left a comment

Choose a reason for hiding this comment

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

/lgtm

Thanks!!

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 21, 2020
@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: lilic, paulfantom

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-bot
Copy link
Contributor

/retest

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

@paulfantom paulfantom mentioned this pull request Jul 21, 2020
Merged
@openshift-bot
Copy link
Contributor

/retest

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

13 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-merge-robot openshift-merge-robot merged commit aa932e8 into openshift:master Jul 22, 2020
@paulfantom paulfantom deleted the client-go branch July 22, 2020 07:34
@openshift-ci-robot openshift-ci-robot mentioned this pull request Jul 22, 2020
Merged
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@simonpasquier simonpasquier simonpasquier left review comments

@lilic lilic lilic approved these changes

@s-urbaniak s-urbaniak Awaiting requested review from s-urbaniak

Assignees

@lilic lilic

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.

None yet
6 participants
@paulfantom @lilic @openshift-ci-robot @openshift-bot @simonpasquier @openshift-merge-robot