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.

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

Bug 1874056: Clean up and modify to use library-go verify package #535

Merged
merged 2 commits into from Sep 16, 2020

Conversation

jottofar
Copy link
Contributor

Modify to use proper k8s encoding/decoding and other minor clean up. Reference open comments from #343. Use library-go verify package which was created since both CVO and the oc client use them.

@jottofar
Copy link
Contributor Author

/test unit

@jottofar
Copy link
Contributor Author

/retest

@jottofar
Copy link
Contributor Author

jottofar commented Sep 9, 2020

/retest

2 similar comments
@damemi
Copy link

damemi commented Sep 9, 2020

/retest

@jottofar
Copy link
Contributor Author

jottofar commented Sep 9, 2020

/retest

@jottofar
Copy link
Contributor Author

/hold
for more testing

@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 Sep 10, 2020
@jottofar
Copy link
Contributor Author

/test e2e-cmd

@damemi
Copy link

damemi commented Sep 10, 2020

@soltysh these failures look like infra, not related to these changes right?

looks like timeouts:

I0909 22:43:16.785717     401 helpers.go:234] Connection error: Get https://api.ci-op-mlhn684d-d5f1e.origin-ci-int-gce.dev.openshift.com:6443/api/v1/namespaces/cmd-services/services/mynodeport: dial tcp 10.0.0.3:6443: i/o timeout
F0909 22:43:16.785742     401 helpers.go:115] Unable to connect to the server: dial tcp 10.0.0.3:6443: i/o timeout
goroutine 1 [running]:
k8s.io/klog/v2.stacks(0xc000010001, 0xc0007d2000, 0x73, 0x1cc)
	/go/src/github.com/openshift/oc/vendor/k8s.io/klog/v2/klog.go:996 +0xb8
k8s.io/klog/v2.(*loggingT).output(0x51dab80, 0xc000000003, 0x0, 0x0, 0xc0015dcb60, 0x4d64e92, 0xa, 0x73, 0x419700)
	/go/src/github.com/openshift/oc/vendor/k8s.io/klog/v2/klog.go:945 +0x19d
k8s.io/klog/v2.(*loggingT).printDepth(0x51dab80, 0x3, 0x0, 0x0, 0x2, 0xc00191f9e0, 0x1, 0x1)
	/go/src/github.com/openshift/oc/vendor/k8s.io/klog/v2/klog.go:718 +0x15e
k8s.io/klog/v2.FatalDepth(...)

@jottofar
Copy link
Contributor Author

@damemi @soltysh Thanks for looking. I thought they were infra and I've seen lots of former discussion on slack about them as well but nor recent, e.g. https://coreos.slack.com/archives/CEKNRGF25/p1586344512372800

Copy link
Member

@soltysh soltysh left a comment

Choose a reason for hiding this comment

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

Your bump seems incomplete, b/c it's missing go.sum being modified. Please re-run go mod tidy pointing library-go to master and it should pick all the necessary changes. It's also possible that the necessary library-go changes landed already during recent bumps.

@damemi
Copy link

damemi commented Sep 14, 2020

It's also possible that the necessary library-go changes landed already during recent bumps.

@soltysh I think this is the case, because I was talking with @jottofar offline about this PR before we got the 1.19 bump in place. Wouldn't hurt to double-check though

@jottofar
Copy link
Contributor Author

jottofar commented Sep 14, 2020

@soltysh Yeah, I picked up the changes to go.mod from @damemi:

$ git tattle go.mod
...
0384c91 (Maciej Szulik 2020-02-19 11:34:48 +0100 41) github.com/mtrmac/gpgme v0.1.2 // indirect
e4de4fa (Maciej Szulik 2019-09-22 13:37:21 +0200 42) github.com/opencontainers/go-digest v1.0.0-rc1
91ed2df (Mike Dame 2020-09-08 08:46:30 -0400 43) github.com/openshift/api v0.0.0-20200901182017-7ac89ba6b971
91ed2df (Mike Dame 2020-09-08 08:46:30 -0400 44) github.com/openshift/build-machinery-go v0.0.0-20200819073603-48aa266c95f7
91ed2df (Mike Dame 2020-09-08 08:46:30 -0400 45) github.com/openshift/client-go v0.0.0-20200827190008-3062137373b5
91ed2df (Mike Dame 2020-09-08 08:46:30 -0400 46) github.com/openshift/library-go v0.0.0-20200907120738-ea57b121ba1a
...
$ git log
commit 74a8213 (HEAD -> ota-193, origin/ota-193)
Author: Jack Ottofaro jottofar@redhat.com
Date: Wed Sep 9 11:45:07 2020 -0400

Clean up and modify to use library-go verify package

Modify to use proper k8s encoding/decoding and other minor cleanup.
Reference open comments from #343.
Use library-go verify package which was created since both CVO and
the oc client use them.

commit b90dcf9
Author: Jack Ottofaro jottofar@redhat.com
Date: Wed Sep 9 11:42:16 2020 -0400

Bump library-go to pick up new verify package

Did:

$ go mod tidy
$ go mod vendor

commit 20df740 (upstream/master, origin/master, origin/HEAD, master)
Merge: 40679be 4e35f1e
Author: OpenShift Merge Robot openshift-merge-robot@users.noreply.github.com
Date: Wed Sep 9 09:18:47 2020 -0400

Merge pull request #547 from damemi/oc-4.6-k8s-1.19-rebase

Bug 1874056: Rebase k8s to 1.19.0

commit 40679be (upstream/release-4.7, upstream/release-4.6)

@jottofar
Copy link
Contributor Author

/unhold

@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 Sep 14, 2020
@soltysh
Copy link
Member

soltysh commented Sep 15, 2020

@jottofar still you're missing go.sum modifications, this file holds check sums for all the dependencies go.mod resolved.

@soltysh
Copy link
Member

soltysh commented Sep 15, 2020

Compare #567, for example.

@jottofar
Copy link
Contributor Author

@soltysh gotcha. Believe I have it right now in commit 06d1694.

Copy link
Member

@soltysh soltysh left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve

@openshift-ci-robot openshift-ci-robot added 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 Sep 15, 2020
@jottofar
Copy link
Contributor Author

/test build-rpms-from-tar

@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.

8 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.

Did:

$ go mod tidy
$ go mod vendor
Modify to use proper k8s encoding/decoding and other minor cleanup.
Reference open comments from openshift#343.
Use library-go verify package which was created since both CVO and
the oc client use them.
@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Sep 15, 2020
@jottofar
Copy link
Contributor Author

@soltysh #567 merged so I had to rebase

Copy link
Member

@soltysh soltysh left a comment

Choose a reason for hiding this comment

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

/lgtm

@soltysh soltysh changed the title Clean up and modify to use library-go verify package Bug 1874056: Clean up and modify to use library-go verify package Sep 16, 2020
@openshift-ci-robot openshift-ci-robot added lgtm Indicates that a PR is ready to be merged. bugzilla/severity-urgent Referenced Bugzilla bug's severity is urgent for the branch this PR is targeting. bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. labels Sep 16, 2020
@openshift-ci-robot
Copy link

@jottofar: This pull request references Bugzilla bug 1874056, which is valid. The bug has been updated to refer to the pull request using the external bug tracker.

3 validation(s) were run on this bug
  • bug is open, matching expected state (open)
  • bug target release (4.6.0) matches configured target release for branch (4.6.0)
  • bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

In response to this:

Bug 1874056: Clean up and modify to use library-go verify package

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
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jottofar, soltysh

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.

1 similar comment
@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 e973b42 into openshift:master Sep 16, 2020
@openshift-ci-robot
Copy link

@jottofar: Some pull requests linked via external trackers have merged:

The following pull requests linked via external trackers have not merged:

These pull request must merge or be unlinked from the Bugzilla bug in order for it to move to the next state.

Bugzilla bug 1874056 has not been moved to the MODIFIED state.

In response to this:

Bug 1874056: Clean up and modify to use library-go verify package

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.

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. bugzilla/severity-urgent Referenced Bugzilla bug's severity is urgent for the branch this PR is targeting. bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants