Skip to content

Move metrics e2e to one namespace per spec #2716

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

Conversation

perdasilva
Copy link
Collaborator

Signed-off-by: perdasilva [email protected]

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

Motivation for the change:
CI 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 openshift-ci bot requested review from anik120 and joelanford March 27, 2022 09:22
@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 27, 2022
@perdasilva perdasilva force-pushed the refactor_metrics_e2e branch from 6d091a2 to 75ec818 Compare March 27, 2022 09:24

AfterEach(func() {
TeardownNamespace(generatedNamespace.GetName())
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 BeforeEach that's nested in the first Context container (lines 53-55) still relevant now:

		BeforeEach(func() {
			By("using the default OperatorGroup created in BeforeSuite")
		})

Copy link
Member

@timflannagan timflannagan left a comment

Choose a reason for hiding this comment

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

It looks like there's a test case that creates a failing CSV without an explicit metadata.Namespace but I think that's fine as the test case does properly cleanup that CSV afterwards.

Ran this locally and didn't see any obvious resource leakage:

$ gh pr checkout 2716
$ kind create cluster ; make run-local
$ make e2e E2E_INSTALL_NS=olm TEST="Metrics are generated for OLM managed resources" 
$  k get ns
NAME                 STATUS   AGE
default              Active   10m
kube-node-lease      Active   11m
kube-public          Active   11m
kube-system          Active   11m
local-path-storage   Active   10m
olm                  Active   9m14s
operators            Active   9m14s
$ k get crds
NAME                                          CREATED AT
catalogsources.operators.coreos.com           2022-03-28T15:34:48Z
clusterserviceversions.operators.coreos.com   2022-03-28T15:34:48Z
installplans.operators.coreos.com             2022-03-28T15:34:50Z
olmconfigs.operators.coreos.com               2022-03-28T15:34:51Z
operatorconditions.operators.coreos.com       2022-03-28T15:34:52Z
operatorgroups.operators.coreos.com           2022-03-28T15:34:52Z
operators.operators.coreos.com                2022-03-28T15:34:54Z
subscriptions.operators.coreos.com            2022-03-28T15:34:55Z
$ k get operators
NAME                      AGE
myapp.metrics-e2e-29wqp   4m16s
myapp.metrics-e2e-7bnpt   4m17s
myapp.metrics-e2e-gtpjc   4m17s
myapp.metrics-e2e-jnzfz   4m17s
myapp.metrics-e2e-t7blg   4m15s

@timflannagan
Copy link
Member

Holding in case we need a second reviewer. Feel free to remove the hold.

/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
@perdasilva perdasilva force-pushed the refactor_metrics_e2e branch from 75ec818 to eee9aea Compare March 28, 2022 17:35
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Mar 28, 2022
@perdasilva perdasilva force-pushed the refactor_metrics_e2e branch from eee9aea to 97e3f8c Compare March 29, 2022 07:43
@perdasilva perdasilva force-pushed the refactor_metrics_e2e branch from 97e3f8c to 43dd6ce Compare March 29, 2022 13:43
@perdasilva
Copy link
Collaborator Author

/lgtm

@openshift-ci
Copy link

openshift-ci bot commented Mar 29, 2022

@perdasilva: you cannot LGTM your own PR.

In response to this:

/lgtm

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.

Copy link
Member

@timflannagan timflannagan left a comment

Choose a reason for hiding this comment

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

/lgtm
/hold cancel

@openshift-ci openshift-ci bot added lgtm Indicates that a PR is ready to be merged. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Mar 29, 2022
@openshift-ci
Copy link

openshift-ci bot commented Mar 29, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: perdasilva, timflannagan

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:
  • OWNERS [perdasilva,timflannagan]

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

@openshift-merge-robot openshift-merge-robot merged commit 51a2097 into operator-framework:master Mar 29, 2022
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. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants