Bug 1903165: NE-199 Follow Up Fixes. #501

sgreene570 · 2020-12-03T21:40:17Z

This PR addresses several fixups from NE-199 Phase 1 and Phase 2. The most notable being the redundant e2e test code in test/e2e/canary_test.go (see the BZ 1903165).

Previous PR comments that this PR addresses (in addition to the canary status e2e test cleanup):

#493 (comment)
https://github.com/openshift/cluster-ingress-operator/pull/476/files#r513779823
https://github.com/openshift/cluster-ingress-operator/pull/476/files#r513784808.

See individual commit messages for more details.

openshift-ci-robot · 2020-12-03T21:40:24Z

@sgreene570: This pull request references Bugzilla bug 1903165, 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)

Details

In response to this:

Bug 1903165: NE-199 Follow Up Fixes.

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 · 2020-12-03T21:40:44Z

@sgreene570: This pull request references Bugzilla bug 1903165, which is valid.

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 POST, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

Details

In response to this:

Bug 1903165: NE-199 Follow Up Fixes.

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 · 2020-12-03T21:41:17Z

@sgreene570: This pull request references Bugzilla bug 1903165, which is valid.

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 POST, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

Details

In response to this:

Bug 1903165: NE-199 Follow Up Fixes.

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.

sgreene570 · 2020-12-04T01:04:20Z

/retest

Move the Default<Operator/Operand/Canary>Namespace constant definitions from pkg/manifests/manifests.go to pkg/operator/controller/names.go. Correct any invalid references to Default<Foo>Namespace in the process and also replace any hard-coded namespace strings with the correct constant in names.go

pkg/operator/controller/<canary,dns,ingress,status>: Make each controller's `Config` field in `reconciler` types private, since they do not need to be exported. Fix any references to the newly private `config` field of each `reconciler` type.

Remove redundant kubeClient in canary tests: TestMain in test/e2e/operator_test.go defines a global `kclient`, so there's no need for test/e2e/canary_test.go to create another. Move canary status condition test: Simplify the canary status condition test for the default ingress controller by moving the test logic from test/e2e/canary_test.go to test/e2e/operator_test.go. Modify the existing default ingress status test to check for the canary status condition.

pkg/operator/controller/names.go: Shorten the canary route name so that the canary route hostname isn't needlessly long. pkg/operator/controller/canary/route_test.go: Update route unit tests to match name change.

sgreene570 · 2020-12-08T14:11:21Z

/retest

sgreene570 · 2020-12-08T16:27:05Z

/retest

frobware · 2020-12-08T16:41:59Z

/lgtm

openshift-ci-robot · 2020-12-08T16:42:18Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: frobware, sgreene570

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~~ [frobware,sgreene570]

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

openshift-ci-robot · 2020-12-08T17:46:54Z

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

openshift/cluster-ingress-operator#501

Bugzilla bug 1903165 has been moved to the MODIFIED state.

Details

In response to this:

Bug 1903165: NE-199 Follow Up Fixes.

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.

Miciah

A belated review with suggestions for a few more possible follow-ups...

Miciah · 2020-12-31T07:50:47Z

pkg/operator/controller/certificate/controller.go

 	// TODO if network-edge wishes to expand the scope of the CA bundle (and you could legitimately see a need/desire to have one CA that verifies all ingress traffic).
 	// TODO this could be accomplished using union logic similar to the kube-apiserver's join of multiple CAs.
-	if ingress == nil || ingress.Namespace != "openshift-ingress-operator" || ingress.Name != "default" {
+	if ingress == nil || ingress.Namespace != operatorcontroller.DefaultOperatorNamespace || ingress.Name != "default" {


We should also move DefaultIngressControllerName to pkg/operator/controller/names.go and use operatorcontroller.DefaultIngressControllerName here.

Noted. Thanks!

Miciah · 2020-12-31T07:51:08Z

pkg/operator/controller/ingress/controller.go

+	if err := r.cache.List(context.TODO(), pods, client.InNamespace(operatorcontroller.DefaultOperandNamespace)); err != nil {
+		errs = append(errs, fmt.Errorf("failed to list pods in namespace %q: %v", operatorcontroller.DefaultOperatorNamespace, err))


Suggested change

if err := r.cache.List(context.TODO(), pods, client.InNamespace(operatorcontroller.DefaultOperandNamespace)); err != nil {

errs = append(errs, fmt.Errorf("failed to list pods in namespace %q: %v", operatorcontroller.DefaultOperatorNamespace, err))

if err := r.cache.List(context.TODO(), pods, client.InNamespace(operatorcontroller.DefaultOperandNamespace)); err != nil {

errs = append(errs, fmt.Errorf("failed to list pods in namespace %q: %v", operatorcontroller.DefaultOperandNamespace, err))

whoops. Thanks for catching this!

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 3, 2020

openshift-ci-robot requested review from Miciah and ironcladlou December 3, 2020 21:40

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 3, 2020

sgreene570 added 3 commits December 7, 2020 13:26

Make reconciler Config fields private

1de8597

pkg/operator/controller/<canary,dns,ingress,status>: Make each controller's `Config` field in `reconciler` types private, since they do not need to be exported. Fix any references to the newly private `config` field of each `reconciler` type.

sgreene570 force-pushed the NE-199-follow-ups branch from 18530d8 to 1ebdd4c Compare December 7, 2020 18:49

controller/canary: Shorten canary route name

472bb12

pkg/operator/controller/names.go: Shorten the canary route name so that the canary route hostname isn't needlessly long. pkg/operator/controller/canary/route_test.go: Update route unit tests to match name change.

sgreene570 force-pushed the NE-199-follow-ups branch from 1ebdd4c to 472bb12 Compare December 7, 2020 19:54

openshift-ci-robot assigned frobware Dec 8, 2020

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Dec 8, 2020

openshift-merge-robot merged commit 48dd8e1 into openshift:master Dec 8, 2020

Miciah reviewed Dec 31, 2020

View reviewed changes

		if err := r.cache.List(context.TODO(), pods, client.InNamespace(operatorcontroller.DefaultOperandNamespace)); err != nil {
		errs = append(errs, fmt.Errorf("failed to list pods in namespace %q: %v", operatorcontroller.DefaultOperatorNamespace, err))

Bug 1903165: NE-199 Follow Up Fixes. #501

Bug 1903165: NE-199 Follow Up Fixes. #501

Uh oh!

Conversation

sgreene570 commented Dec 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openshift-ci-robot commented Dec 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openshift-ci-robot commented Dec 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openshift-ci-robot commented Dec 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sgreene570 commented Dec 4, 2020

Uh oh!

sgreene570 commented Dec 8, 2020

Uh oh!

sgreene570 commented Dec 8, 2020

Uh oh!

frobware commented Dec 8, 2020

Uh oh!

openshift-ci-robot commented Dec 8, 2020

Uh oh!

openshift-ci-robot commented Dec 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Miciah left a comment

Choose a reason for hiding this comment

Uh oh!

Miciah Dec 31, 2020

Choose a reason for hiding this comment

Uh oh!

sgreene570 Jan 4, 2021

Choose a reason for hiding this comment

Uh oh!

Miciah Dec 31, 2020

Choose a reason for hiding this comment

Uh oh!

sgreene570 Jan 4, 2021

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

sgreene570 commented Dec 3, 2020 •

edited

Loading

openshift-ci-robot commented Dec 3, 2020 •

edited

Loading

openshift-ci-robot commented Dec 3, 2020 •

edited

Loading

openshift-ci-robot commented Dec 3, 2020 •

edited

Loading

openshift-ci-robot commented Dec 8, 2020 •

edited

Loading