Skip to content

test/: clean up some e2e setup code #4315

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

Merged
merged 1 commit into from
Dec 15, 2020

Conversation

estroz
Copy link
Member

@estroz estroz commented Dec 15, 2020

Description of the change: clean up some e2e setup code

Motivation for the change: fix some potential panics and warn on error instead of not checking error at all

Checklist

If the pull request includes user-facing changes, extra documentation is required:


By("cleaning up permissions")
_, _ = tc.Kubectl.Command("delete", "clusterrolebinding",
metricsClusterRoleBindingName)
Copy link
Contributor

@camilamacedo86 camilamacedo86 Dec 15, 2020

Choose a reason for hiding this comment

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

the reason. for the break in the lines is for not be required to scroll to check the code.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think these lines I changed should still be < 120 chars wide.


By("deploying project on the cluster")
err := tc.Make("deploy", "IMG="+tc.ImageName)
Expect(err).NotTo(HaveOccurred())
Expect(tc.Make("deploy", "IMG="+tc.ImageName)).To(Succeed())
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

// WrapWarn is a one-liner to wrap an error from a command that returns (error) in a warning.
func WrapWarn(err error) {
WrapWarnOutput("", err)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

WDYT. about push it to upstream?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure!

Copy link
Contributor

@camilamacedo86 camilamacedo86 left a comment

Choose a reason for hiding this comment

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

I added a few comments.
However, I am ok with it. I do not think that the test should fail because any flake faced to cleanup the env as well. 👍

/lgtm

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

@jmrodri jmrodri left a comment

Choose a reason for hiding this comment

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

/lgtm

@estroz estroz merged commit ef19706 into operator-framework:master Dec 15, 2020
@estroz estroz deleted the chore/e2e-cleanup branch December 15, 2020 08:39
reinvantveer pushed a commit to reinvantveer/operator-sdk that referenced this pull request Feb 5, 2021
rearl-scwx pushed a commit to rearl-scwx/operator-sdk that referenced this pull request Feb 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants