CFE-1108: Use upstream e2e to run using downstream images #11

arkadeepsen · 2024-08-12T07:37:08Z

Description of the change:

This PR uses upstream e2e to run using downstream images

Motivation for the change:

Add support for running e2e downstream

Corresponding release PR: openshift/release#55440

Add RBAC permissions to create a project Add option to skip generation of testdata related to secrets Add option to skip local run in e2e Add option to skip image build and use pre-built memcached-molecule operator image Add security context to the spec of the pod used for fetching metrics Report a warning instead of test failure if undeply of memcached-molecule operator fails Add make target for only running the e2e test

…perator

openshift-ci-robot · 2024-08-12T07:37:12Z

@arkadeepsen: This pull request references CFE-1100 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.18.0" version, but no target version was set.

In response to this:

Description of the change:

This PR uses upstream e2e to run using downstream images

Motivation for the change:

Add support for running e2e downstream

Corresponding release PR: openshift/release#55440

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 openshift-eng/jira-lifecycle-plugin repository.

openshift-ci · 2024-08-12T07:38:41Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: arkadeepsen

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~~ [arkadeepsen]

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

arkadeepsen · 2024-08-12T07:42:08Z

/hold
until we get a green signal in the new e2e CI job

openshift-ci · 2024-08-12T07:48:10Z

@arkadeepsen: all tests passed!

Full PR test history. Your PR dashboard.

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-sigs/prow repository. I understand the commands that are listed here.

chiragkyal

Adding few comments, I'll take another look.

chiragkyal · 2024-08-12T08:16:14Z

Makefile

 test-e2e-ansible-molecule:: install dev-install image/ansible-operator ## Run molecule-based Ansible e2e tests
 	go run ./hack/generate/samples/molecule/generate.go
 	./hack/tests/e2e-ansible-molecule.sh


we should also try adding test-e2e-ansible-molecule target in the CI, as a followup PR may be.

We can take a look at it.

chiragkyal · 2024-08-12T08:22:04Z

Makefile

+.PHONY: test-e2e-ansible-run
+test-e2e-ansible-run:
 	go test ./test/e2e/ansible -v -ginkgo.v


I think it's better to move this target above # Double colon rules allow repeated rule declarations. comment line for better flow understanding.

I thought adding the test-e2e-ansible-run target just below the test-e2e-ansible target would make it easier to understand the flow.

chiragkyal · 2024-08-12T08:44:41Z

hack/generate/samples/ansible/generate.go

 	Kind:    "Memcached",
 }

+var skipSecretGeneration = ""


Is there a reason to make it a global variable? If this variable is getting used only in GenerateMoleculeSample, we can make it local to that function to avoid unexpected use by other files inside this package.

I didn't want to make changes to the existing function definitions. That's why I used the global variable.

chiragkyal · 2024-08-12T08:47:26Z

hack/generate/samples/ansible/generate.go

+		pkg.CheckError("generating ansible molecule sample - ignore", err)
+	}

 	ImplementMemcached(ansibleMoleculeMemcached, bundleImage)


ImplementMemcached can take skipSecretGeneration as an arg to avoid relying on global variable.

Same as #11 (comment).

chiragkyal · 2024-08-12T10:01:08Z

hack/generate/samples/ansible/generate.go

-		samplecli.WithName(ansibleMoleculeMemcached.Name()),
-	)
-	pkg.CheckError("creating ignore samples", err)
+	skipSecretGeneration = os.Getenv("SKIP_SECRET_GENERATION")


skipSecretGeneration would hold only boolean values, and the default should be false. Would it make sense to update the type to bool? The following helper functions could be useful.

// env returns an environment variable or a default value if not specified. func env(key string, defaultValue string) string { val := os.Getenv(key) if len(val) == 0 { return defaultValue } return val } func isTrue(s string) bool { v, _ := strconv.ParseBool(s) return v }

Suggested change

skipSecretGeneration = os.Getenv("SKIP_SECRET_GENERATION")

skipSecretGeneration := isTrue(env("SKIP_SECRET_GENERATION", "")

The usage of strconv.ParseBool() will introduce the issue of error handling. The idea is to add code with minimal/no chances of erroring out. The current code simply says that if the SKIP_SECRET_GENERATION env var is not set to any value, then the operation will work as it is working currently. However, if you need to skip the generation of the secret related code, then set the env var to any value (need not be boolean). This avoids the additional checking of the value being a valid boolean or not.

chiragkyal · 2024-08-12T10:49:52Z

Makefile

SKIP_SECRET_GENERATION ENV can be exported with false

Explained in #11 (comment).

chiragkyal · 2024-08-12T10:56:19Z

openshift/Dockerfile.e2e-ansible

@@ -0,0 +1,20 @@
+FROM basebuilder AS builder


Should we directly use the image tag to avoid overwriting in release config?

It seems easier to maintain it like this. We won't have to update it on every branch cut and it will minimize the effort going forward. Also we just need the basebulider to re-generate the testdata directory. We are not building any binary which will be impacted by the base image.

chiragkyal · 2024-08-12T11:04:59Z

test/e2e/ansible/cluster_test.go


 			By("cleaning up created API objects during test process")
-			Expect(operator.UndeployOperator(ansibleSample)).To(Succeed())
+			testutils.WrapWarnOutput("", operator.UndeployOperator(ansibleSample))


Is Undeploy causing some issues? Currently, there is only one It, so it's okay to use Warn instead of Error, but if the test grows, it could be an issue, and I think upstream won't accept this change.

The make undeploy removes the objects in the same order they are created. The namespace is deleted first and as it gets deleted all the resources created in the namespaces also gets deleted along with it. This always fails when running in an OCP cluster. However, on a kind cluster, this doesn't fail as the deletion of namespace takes some time and thus the make undeploy completes successfully.

I think it's okay to not fail a test if the cleanup fails. It's not related to the actual e2e test anyway.

chiragkyal · 2024-08-12T11:09:46Z

testdata/memcached-molecule-operator/config/rbac/role.yaml

The changes should happen in runtime inside the container, do we need to commit the changes?

This code is updated as a result of running make generate. This additional permission is anyway required and it was kind of a bug not to include the permissions in the first place.

Also if we don't run make generate before pushing the changes to the PR, it'll fail the sanity check.

chiragkyal · 2024-08-12T11:09:59Z

testdata/memcached-molecule-operator/molecule/default/tasks/memcached_test.yml

Same as above

Again, this is updated as a result of running make generate. If we don't run make generate before pushing the changes to the PR, it'll fail the sanity check.

openshift-ci-robot · 2024-08-13T16:23:25Z

@arkadeepsen: This pull request references CFE-1108 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the sub-task to target the "4.18.0" version, but no target version was set.

In response to this:

Description of the change:

This PR uses upstream e2e to run using downstream images

Motivation for the change:

Add support for running e2e downstream

Corresponding release PR: openshift/release#55440

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 openshift-eng/jira-lifecycle-plugin repository.

TrilokGeer · 2024-08-14T06:41:53Z

testdata/memcached-molecule-operator/config/rbac/role.yaml

+  ## Apply customize roles related to project.openshift.io
+  ##
+  - apiGroups:
+      - project.openshift.io


Are the api groups installed via a CR on kind cluster?

TrilokGeer · 2024-08-14T06:45:28Z

@arkadeepsen, some changes assume roles that are specific to openshift apis. Have this PR been tested with kind cluster deployed by default in the Makefile?

openshift-bot · 2024-11-12T09:00:48Z

Issues go stale after 90d of inactivity.

Mark the issue as fresh by commenting /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.
Exclude this issue from closing by commenting /lifecycle frozen.

If this issue is safe to close now please do so with /close.

/lifecycle stale

openshift-bot · 2024-12-13T00:30:32Z

Stale issues rot after 30d of inactivity.

Mark the issue as fresh by commenting /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.
Exclude this issue from closing by commenting /lifecycle frozen.

If this issue is safe to close now please do so with /close.

/lifecycle rotten
/remove-lifecycle stale

openshift-bot · 2025-01-12T08:00:34Z

Rotten issues close after 30d of inactivity.

Reopen the issue by commenting /reopen.
Mark the issue as fresh by commenting /remove-lifecycle rotten.
Exclude this issue from closing again by commenting /lifecycle frozen.

/close

openshift-ci · 2025-01-12T08:00:50Z

@openshift-bot: Closed this PR.

In response to this:

Rotten issues close after 30d of inactivity.

Reopen the issue by commenting /reopen.
Mark the issue as fresh by commenting /remove-lifecycle rotten.
Exclude this issue from closing again by commenting /lifecycle frozen.

/close

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-sigs/prow repository.

arkadeepsen added 2 commits August 10, 2024 21:40

UPSTREAM: <carry>: Add downstream Dockerfile for memcached-molecule-o…

c6d4fe0

…perator

openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Aug 12, 2024

openshift-ci bot requested review from TrilokGeer and rashmigottipati August 12, 2024 07:38

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 12, 2024

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

chiragkyal reviewed Aug 12, 2024

View reviewed changes

arkadeepsen changed the title ~~CFE-1100: Use upstream e2e to run using downstream images~~ CFE-1108: Use upstream e2e to run using downstream images Aug 13, 2024

TrilokGeer reviewed Aug 14, 2024

View reviewed changes

chiragkyal mentioned this pull request Aug 14, 2024

CFE-1110: UPSTREAM: <carry>: Bump community.docker to 3.10.3 #13

Merged

openshift-ci bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Nov 12, 2024

openshift-ci bot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Dec 13, 2024

openshift-ci bot closed this Jan 12, 2025

	skipSecretGeneration = os.Getenv("SKIP_SECRET_GENERATION")
	skipSecretGeneration := isTrue(env("SKIP_SECRET_GENERATION", "")

CFE-1108: Use upstream e2e to run using downstream images #11

CFE-1108: Use upstream e2e to run using downstream images #11

Uh oh!

Conversation

arkadeepsen commented Aug 12, 2024

Uh oh!

openshift-ci-robot commented Aug 12, 2024 • edited by openshift-ci bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openshift-ci bot commented Aug 12, 2024

Uh oh!

arkadeepsen commented Aug 12, 2024

Uh oh!

openshift-ci bot commented Aug 12, 2024

Uh oh!

chiragkyal left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

openshift-ci-robot commented Aug 13, 2024 • edited by openshift-ci bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TrilokGeer commented Aug 14, 2024

Uh oh!

openshift-bot commented Nov 12, 2024

Uh oh!

openshift-bot commented Dec 13, 2024

Uh oh!

openshift-bot commented Jan 12, 2025

Uh oh!

openshift-ci bot commented Jan 12, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

openshift-ci-robot commented Aug 12, 2024 •

edited by openshift-ci bot

Loading

openshift-ci-robot commented Aug 13, 2024 •

edited by openshift-ci bot

Loading