Ensure the $ARTIFACT_DIR variable is respected #2721

timflannagan · 2022-03-29T19:14:58Z

Description of the change:
#2432 had initially added support for CI to gather testing artifacts when the $ARTIFACT_DIR (#2729 renamed this variable from $ARTIFACTS_DIR from the original implementation) was specified, and the kind provisioner was responsible for running a bash script that fired off a series of kubectl commands at the namespace level. This functionality works fine for CI environments that are configured to use the make e2e-local target, but this kind of functionality wasn't emulated to also work with the kubeconfig provisioner that's used under-the-hood when users run make e2e directly.

These changes aim to consolidate both of those provisioner implementations so that both of them gathering testing artifacts when that variable has been specified, and an individual test case has failed.

Motivation for the change:
Ensure both the kubeconfig and kind provisioner(s) are able to collecting testing artifacts.

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 · 2022-03-29T19:15:03Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 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~~ [timflannagan]

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

timflannagan · 2022-03-29T19:19:23Z

test/e2e/ctx/ctx.go

@@ -116,8 +117,7 @@ func (ctx TestContext) DumpNamespaceArtifacts(namespace string) error {
 		"KUBECONFIG=" + kubeconfigPath,
 	}

-	// compiled test binary running e2e tests is run from the root ./bin directory
-	cmd := exec.Command("../test/e2e/collect-ci-artifacts.sh")


I ran into a sourcing issue when running the make e2e command locally as it's working directory points to ./test/e2e, so there's some skew between the working directory when using the kind or kubeconfig provisioners.

timflannagan · 2022-03-29T19:21:08Z

test/e2e/e2e_test.go

+	if collectArtifactsScriptPath != nil && *collectArtifactsScriptPath != "" {
+		os.Setenv("E2E_ARTIFACTS_SCRIPT", *collectArtifactsScriptPath)
+	}


Note: there wasn't any clear way of injecting global state to the ctx package, and there weren't any setter methods so I wanted to avoid introducing that pattern, so we're using an internal environment variable to basically act as a cross-package variable. This is admittedly a bit hacky of an implementation so I'm open to suggestions.

perdasilva · 2022-03-30T06:40:36Z

test/e2e/ctx/ctx.go

@@ -137,6 +137,17 @@ func setDerivedFields(ctx *TestContext) error {
 		return fmt.Errorf("nil RESTClient")
 	}

+	if ctx.artifactsDir == "" {


Could this poorly interact with some downstream only code: https://github.com/openshift/operator-framework-olm/blob/master/staging/operator-lifecycle-manager/test/e2e/e2e_test.go#L69?

We should be in the clear there as we're building up the junit reporter for ginkgo vs. telling either of the provisioner implementation where to dump testing artifacts when configured.

Also: we could bubble up that downstream-only change to make upstream backwards-compatible to eliminate any skew between implementations (which might be relevant for any effort around ginkgo v1 -> v2)?

timflannagan · 2022-04-06T17:41:09Z

I'll try and come back to this at some point.

/hold

timflannagan · 2022-04-07T15:15:06Z

Ran these changes locally after rebasing against master:

$ make e2e ARTIFACT_DIR=./tmp E2E_INSTALL_NS=olm TEST="Not found APIs"
...
$ tree test/e2e/tmp
test/e2e/tmp
├── deprecated-e2e-pvt9s
│   ├── get_catalogsources_-o_yaml
│   ├── get_clusterserviceversions_-o_yaml
│   ├── get_events_--sort-by_.lastTimestamp
│   ├── get_installplans_-o_yaml
│   ├── get_operatorgroups_-o_yaml
│   ├── get_pods_-o_wide
│   └── get_subscriptions_-o_yaml
└── junit
    └── junit_e2e_01.xml

And I had updated one of the test cases in the deprecated e2e suite that's being focused on in that example to purposely fail:

-                               Expect(ctx.Ctx().Client().Status().Update(context.Background(), ip)).To(Succeed(), "failed to update the resource")
+                               Expect(ctx.Ctx().Client().Status().Update(context.Background(), ip)).ToNot(Succeed(), "failed to update the resource")

timflannagan · 2022-04-07T15:16:47Z

test/e2e/e2e_test.go

+	if collectArtifactsScriptPath != nil && *collectArtifactsScriptPath != "" {
+		os.Setenv("E2E_ARTIFACT_SCRIPT", *collectArtifactsScriptPath)
+	}



Note: just a hack to inject state so the ctx package can derive where to find the artifacts bash script path location.

timflannagan · 2022-04-07T15:17:12Z

/hold cancel

dinhxuanvu · 2022-04-07T15:19:26Z

Can we correct the title, commit message and all references to ARTIFACT_DIR given we just merged a PR to correct that?

This directory is populating when the e2e suite is supplied with an output $ARTIFACT_DIR, and the script that's responsible for gathering CI testing artifacts when a test case fails. Signed-off-by: timflannagan <timflannagan@gmail.com>

Signed-off-by: timflannagan <timflannagan@gmail.com>

dinhxuanvu

/lgtm

openshift-ci bot requested review from gallettilance and joelanford March 29, 2022 19:15

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 29, 2022

timflannagan commented Mar 29, 2022

View reviewed changes

perdasilva reviewed Mar 30, 2022

View reviewed changes

openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 6, 2022

timflannagan force-pushed the ci/respect-artifacts-dir branch from 06b234a to ad497aa Compare April 7, 2022 15:06

timflannagan commented Apr 7, 2022

View reviewed changes

openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 7, 2022

timflannagan changed the title ~~Ensure the $ARTIFACTS_DIR variable is respected~~ Ensure the $ARTIFACT_DIR variable is respected Apr 7, 2022

timflannagan added 2 commits April 7, 2022 11:22

.gitignore: Filter out the nested .kube cache

6deef78

This directory is populating when the e2e suite is supplied with an output $ARTIFACT_DIR, and the script that's responsible for gathering CI testing artifacts when a test case fails. Signed-off-by: timflannagan <timflannagan@gmail.com>

Makefile,test: Ensure that the $ARTIFACT_DIR is respected

f82e274

Signed-off-by: timflannagan <timflannagan@gmail.com>

timflannagan force-pushed the ci/respect-artifacts-dir branch from ad497aa to f82e274 Compare April 7, 2022 15:22

dinhxuanvu reviewed Apr 7, 2022

View reviewed changes

openshift-ci bot assigned dinhxuanvu Apr 7, 2022

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Apr 7, 2022

openshift-merge-robot merged commit 3f07f21 into operator-framework:master Apr 7, 2022

timflannagan deleted the ci/respect-artifacts-dir branch April 7, 2022 15:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ensure the $ARTIFACT_DIR variable is respected #2721

Ensure the $ARTIFACT_DIR variable is respected #2721

timflannagan commented Mar 29, 2022 •

edited

Loading

openshift-ci bot commented Mar 29, 2022

timflannagan Mar 29, 2022

timflannagan Mar 29, 2022

perdasilva Mar 30, 2022

timflannagan Mar 30, 2022

timflannagan Mar 30, 2022

timflannagan commented Apr 6, 2022

timflannagan commented Apr 7, 2022

timflannagan Apr 7, 2022

timflannagan commented Apr 7, 2022

dinhxuanvu commented Apr 7, 2022

dinhxuanvu left a comment

Ensure the $ARTIFACT_DIR variable is respected #2721

Ensure the $ARTIFACT_DIR variable is respected #2721

Conversation

timflannagan commented Mar 29, 2022 • edited Loading

openshift-ci bot commented Mar 29, 2022

timflannagan Mar 29, 2022

Choose a reason for hiding this comment

timflannagan Mar 29, 2022

Choose a reason for hiding this comment

perdasilva Mar 30, 2022

Choose a reason for hiding this comment

timflannagan Mar 30, 2022

Choose a reason for hiding this comment

timflannagan Mar 30, 2022

Choose a reason for hiding this comment

timflannagan commented Apr 6, 2022

timflannagan commented Apr 7, 2022

timflannagan Apr 7, 2022

Choose a reason for hiding this comment

timflannagan commented Apr 7, 2022

dinhxuanvu commented Apr 7, 2022

dinhxuanvu left a comment

Choose a reason for hiding this comment

timflannagan commented Mar 29, 2022 •

edited

Loading