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

Bug 1908471: Bump deps k8s 1.20 #1903

Merged
merged 7 commits into from
Dec 22, 2020
Merged

Bug 1908471: Bump deps k8s 1.20 #1903

merged 7 commits into from
Dec 22, 2020

Conversation

awgreene
Copy link
Member

No description provided.

@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 Dec 10, 2020
@openshift-ci-robot
Copy link
Collaborator

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 10, 2020
kevinrizza
kevinrizza reviewed Dec 10, 2020
View reviewed changes
test/e2e/go.mod Outdated Show resolved Hide resolved
@awgreene awgreene force-pushed the bump-deps-k8s-1.20 branch 5 times, most recently from 1012fc1 to 84fe552 Compare December 16, 2020 15:23
@awgreene awgreene marked this pull request as ready for review December 16, 2020 15:27
@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 Dec 16, 2020
@awgreene
Copy link
Member Author

/retest

1 similar comment
@awgreene
Copy link
Member Author

/retest

@awgreene awgreene changed the title Bump deps k8s 1.20 Bug 1908471: Bump deps k8s 1.20 Dec 16, 2020
@openshift-ci-robot openshift-ci-robot added bugzilla/severity-medium Referenced Bugzilla bug's severity is medium 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 Dec 16, 2020
@openshift-ci-robot
Copy link
Collaborator

@awgreene: This pull request references Bugzilla bug 1908471, 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
  • bug is open, matching expected state (open)
  • bug target release (4.7.0) matches configured target release for branch (4.7.0)
  • bug is in the state ASSIGNED, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

In response to this:

Bug 1908471: Bump deps k8s 1.20

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.

@dinhxuanvu dinhxuanvu mentioned this pull request Dec 16, 2020
Merged
5 tasks
@awgreene
Copy link
Member Author

/retest

@awgreene awgreene force-pushed the bump-deps-k8s-1.20 branch 2 times, most recently from b7d285b to 11ad61a Compare December 17, 2020 13:28
@awgreene
Copy link
Member Author

/retest

2 similar comments
@awgreene
Copy link
Member Author

/retest

@awgreene
Copy link
Member Author

/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-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Dec 19, 2020
@awgreene
Copy link
Member Author

/retest

1 similar comment
@awgreene
Copy link
Member Author

/retest

@awgreene
Copy link
Member Author

awgreene commented Dec 21, 2020 •
edited
Loading

/retest

aws console tests were failing due to a community-operators catalog source that was released with limited operators within.

dinhxuanvu
dinhxuanvu reviewed Dec 21, 2020
View reviewed changes
Copy link
Member

@dinhxuanvu dinhxuanvu left a comment

Choose a reason for hiding this comment

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

/lgtm

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

@awgreene: The following tests failed, say /retest to rerun all failed tests:

Test name Commit Details Rerun command
ci/prow/e2e-aws-console-olm 8a31e33 link /test e2e-aws-console-olm
ci/prow/e2e-aws-olm 8a31e33 link /test e2e-aws-olm

Full PR test history. Your PR dashboard.

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. I understand the commands that are listed here.

@openshift-bot
Copy link
Contributor

/retest

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

@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Dec 21, 2020
@awgreene awgreene force-pushed the bump-deps-k8s-1.20 branch 5 times, most recently from 1da7d66 to 43c95c4 Compare December 22, 2020 00:38
@awgreene
Reduce e2e test failure rate
3a08055 
This commit introduces a few changes to the e2e test suite:
1. It reduces the failure rate of the CSV e2e test by allowing the
    operator to reach a succeeded install before searching for resources.
2. An installplan e2e test was failing a diff check against two
    installplans because the operator controller was applying a component
label to one of the installplans. An update was made to still compare
the spec and status of the two installplans but ignores other fields.
3. A test that ensures OLM elevates scoped  permissions to cluster levels
    when the CSV is installed in a global OperatorGroup was not elevating
them  because all of the permissions defined in the role were already
provided by a clusterrole defined in the csv. The test was sped up
significantly by making the rules defined in the CSV's spec.Permissions
field different from those defined in the spec.ClusterPermissions field.
4. The newCSV function was returning a CSV with an incorrect GVK,
   causing garbage collection to fail. The newCSV function now sets the
GVK correctly.
njhale
njhale reviewed Dec 22, 2020
View reviewed changes
go.mod
Comment on lines 71 to 72
// pinned because no tag supports 1.18 yet
// pinned because no tag supports 1.18 yet
sigs.k8s.io/controller-runtime => sigs.k8s.io/controller-runtime v0.5.1-0.20200414221803-bac7e8aaf90a
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.

Is this still true? Do we still need to pin structured-merge-diff for 1.20?

go.mod
sigs.k8s.io/kind v0.7.0
)

replace (
github.com/Azure/go-autorest => github.com/Azure/go-autorest v13.3.2+incompatible
github.com/docker/docker => github.com/moby/moby v0.7.3-0.20190826074503-38ab9da00309 // Required by Helm
github.com/googleapis/gnostic => github.com/googleapis/gnostic v0.4.1

// controller runtime
github.com/openshift/api => github.com/openshift/api v0.0.0-20200331152225-585af27e34fd // release-4.5
Copy link
Member

Choose a reason for hiding this comment

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

Can we bump our openshift dependencies to a newer release?

pkg/controller/operators/adoption_controller.go Show resolved Hide resolved
pkg/controller/operators/adoption_controller.go Show resolved Hide resolved
pkg/controller/operators/adoption_controller.go Show resolved Hide resolved
pkg/controller/operators/adoption_controller.go
@@ -335,7 +344,11 @@ func (r *AdoptionReconciler) adoptees(ctx context.Context, operator decorators.O
}
opt := client.MatchingLabelsSelector{Selector: selector}
for _, list := range componentLists {
if err := r.List(ctx, list, opt); err != nil {
cList, ok := list.(client.ObjectList)
Copy link
Member

Choose a reason for hiding this comment

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

Can componentLists just be of type []client.Object from the get-go?

test/e2e/csv_e2e_test.go Show resolved Hide resolved
test/e2e/installplan_e2e_test.go Show resolved Hide resolved
test/e2e/installplan_e2e_test.go
@@ -2206,6 +2212,7 @@ var _ = Describe("Install Plan", func() {
}
return true, nil
})
require.NoError(GinkgoT(), err)
Copy link
Member

Choose a reason for hiding this comment

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

Might as well switch over to Eventually here as well, no?

test/e2e/installplan_e2e_test.go Show resolved Hide resolved
@njhale
Copy link
Member

njhale commented Dec 22, 2020 •
edited
Loading

Looking really good. I think we can punt changing function signatures to use controller-runtime's Object interface and address it a follow up, which only leaves the go.mod related comments to address.

@njhale
Copy link
Member

njhale commented Dec 22, 2020

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Dec 22, 2020
@openshift-merge-robot openshift-merge-robot merged commit 72d0608 into operator-framework:master Dec 22, 2020
@openshift-ci-robot
Copy link
Collaborator

@awgreene: All pull requests linked via external trackers have merged:

Bugzilla bug 1908471 has been moved to the MODIFIED state.

In response to this:

Bug 1908471: Bump deps k8s 1.20

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.

@jianzhangbjz jianzhangbjz mentioned this pull request Dec 23, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dinhxuanvu dinhxuanvu dinhxuanvu approved these changes

@kevinrizza kevinrizza kevinrizza left review comments

@njhale njhale njhale left review comments

@ecordell ecordell ecordell left review comments

@gallettilance gallettilance Awaiting requested review from gallettilance

Assignees

@njhale njhale

@dinhxuanvu dinhxuanvu

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. bugzilla/severity-medium Referenced Bugzilla bug's severity is medium 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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@awgreene @openshift-ci-robot @openshift-bot @openshift-merge-robot @njhale @ecordell @kevinrizza @dinhxuanvu