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

Move install plan e2e to one namespace per spec #2708

Merged
merged 1 commit into from
Mar 28, 2022
Merged

Move install plan e2e to one namespace per spec #2708

merged 1 commit into from
Mar 28, 2022

Conversation

perdasilva
Copy link
Collaborator

Signed-off-by: perdasilva perdasilva@redhat.com

Description of the change:
This PR moves the install plan e2e to one namespace per spec

Motivation for the change:
e2e stability

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 /doc
  • Commit messages sensible and descriptive
  • Tests marked as [FLAKE] are truly flaky
  • Tests that remove the [FLAKE] tag are no longer flaky

@openshift-ci
Copy link

openshift-ci bot commented Mar 24, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: perdasilva

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-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 24, 2022
@openshift-ci openshift-ci bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 25, 2022
@openshift-ci openshift-ci bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 25, 2022
@timflannagan
Copy link
Contributor

@perdasilva I'm going to pull this down locally and now and push to your remote if there's any requisite changes that need to be made.

@perdasilva perdasilva force-pushed the refactor_ip_e2e branch 2 times, most recently from 2bd4932 to 2a290de Compare March 26, 2022 08:41
@timflannagan
Copy link
Contributor

timflannagan commented Mar 26, 2022 •
edited
Loading

@perdasilva I had a second to pull down the latest change and it looks like we're still leaking resources. I think I'm at the point with these PRs where we should be more interested in ensuring we aren't leaking namespaces to avoid scope creep with an already large effort. How do you feel about pushing this through, and tracking this resource leakage separately?

Edit: maybe the A/C for this type of effort, at least from the reviewer side-of-things, is to ensure there are zero references to the global testNamespace variable throughout the entire file being changed, on top of ensuring that there's no namespace-level leakage and we'll just have to live with the potential for cluster-scoped test pollution influencing tests over time?

timflannagan
timflannagan reviewed Mar 26, 2022
View reviewed changes
test/e2e/catalog_e2e_test.go
@@ -12,6 +12,8 @@ import (
"time"

operatorsv1 "github.com/operator-framework/api/pkg/operators/v1"
k8serror "k8s.io/apimachinery/pkg/api/errors"
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: it would be nice to standardize import naming conventions at some point. This can be enforced by introducing more configuration through golangci-lint's importas linter like we currently do with rukpak.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Maybe we could add a ticket to the CI epic?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep - sounds reasonable to me.

Copy link
Contributor

Choose a reason for hiding this comment

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

Updated the Epic and added a ticket for this. Opened #2715 since then that tackles that kind of linting enforcement.

timflannagan
timflannagan reviewed Mar 26, 2022
View reviewed changes
test/e2e/catalog_e2e_test.go
csv := newCSV(packageStable, ns.GetName(), "", semver.MustParse("0.1.0"), []apiextensions.CustomResourceDefinition{crd}, nil, nil)

defer func() {
Eventually(func() error {
return ctx.Ctx().KubeClient().ApiextensionsInterface().ApiextensionsV1().CustomResourceDefinitions().Delete(context.TODO(), crd.GetName(), metav1.DeleteOptions{})
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: context.TODO -> context.Background.

Copy link
Contributor

Choose a reason for hiding this comment

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

nit: return client.IgnoreNotFound(...) so we can avoid having to use any complex gomega matching expressions?

Copy link
Contributor

Choose a reason for hiding this comment

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

(I realize I'm making the assumption that this is client utility is still applicable for non-controller-runtime clients)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed. Looking at the implementation of IgnoreNotFound it's exactly what you'd expect lol ^^ so should work with any err

timflannagan
timflannagan reviewed Mar 26, 2022
View reviewed changes
test/e2e/ctx/ctx.go
Comment on lines -165 to 172
apiextensionsv1.AddToScheme,
kscheme.AddToScheme,
operatorsv1alpha1.AddToScheme,
operatorsv1.AddToScheme,
operatorsv2.AddToScheme,
apiextensionsv1.AddToScheme,
apiextensions.AddToScheme,
)
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice catch.

timflannagan
timflannagan reviewed Mar 26, 2022
View reviewed changes
test/e2e/installplan_e2e_test.go
@@ -418,6 +444,10 @@ var _ = Describe("Install Plan", func() {
Succeed(),
WithTransform(k8serrors.IsNotFound, BeTrue()),
))
Expect(ctx.Ctx().Client().Delete(context.Background(), plan)).To(Or(
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: wrap this with client.IgnoreNotFound(...) to avoid having to enforce this through gomega expressions?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm still seeing it's using the gomega expressions?

timflannagan
timflannagan reviewed Mar 26, 2022
View reviewed changes
test/e2e/installplan_e2e_test.go
Comment on lines +2538 to 2540

It("UpdatePreexistingCRDFailed", func() {

Copy link
Contributor

Choose a reason for hiding this comment

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

super off topic: we really need to refactor these top-level It specs (that also deserve a more descriptive name).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

One more ticket for the CI epic =D

@timflannagan
Copy link
Contributor

@perdasilva One more thing: I think this is ready to go from my perspective, but I want to make sure we're aligned with what I had described in #2708 (comment) before we push this through.

@perdasilva
Copy link
Collaborator Author

@perdasilva I had a second to pull down the latest change and it looks like we're still leaking resources. I think I'm at the point with these PRs where we should be more interested in ensuring we aren't leaking namespaces to avoid scope creep with an already large effort. How do you feel about pushing this through, and tracking this resource leakage separately?

Edit: maybe the A/C for this type of effort, at least from the reviewer side-of-things, is to ensure there are zero references to the global testNamespace variable throughout the entire file being changed, on top of ensuring that there's no namespace-level leakage and we'll just have to live with the potential for cluster-scoped test pollution influencing tests over time?

Yeah, I'm good with that. There are tests that might still require the testNamespace. Tests that require the global-operators OG. To your list, I'd only add making sure that those cluster level resources are using genName to keep them unique. But I agree. I think we're good with letting cluster level resource leakage through.

@perdasilva
Move install plan e2e to one namespace per spec
ff414e5 
Signed-off-by: perdasilva <perdasilva@redhat.com>
@timflannagan
Copy link
Contributor

Holding just in case we need another reviewer.

/lgtm
/hold

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 28, 2022
@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Mar 28, 2022
dinhxuanvu
dinhxuanvu reviewed Mar 28, 2022
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

@perdasilva perdasilva removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 28, 2022
@openshift-merge-robot openshift-merge-robot merged commit eb01127 into operator-framework:master Mar 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@timflannagan timflannagan timflannagan left review comments

@dinhxuanvu dinhxuanvu dinhxuanvu left review comments

@gallettilance gallettilance Awaiting requested review from gallettilance

@njhale njhale Awaiting requested review from njhale

Assignees

@dinhxuanvu dinhxuanvu

@timflannagan timflannagan

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.
4 participants
@perdasilva @timflannagan @dinhxuanvu @openshift-merge-robot