Bug 1992596: e2e/cli: Move annotations.sh and basicresources.sh to proper e2e #26415

deejross · 2021-08-20T14:56:45Z

There is a lot in basicresources.sh including non-resource and multi-resource tests. This PR is moving that logic out of the old bash script and into Go tests, while appropriately organizing them.

openshift-ci · 2021-08-20T14:56:50Z

@deejross: This pull request references Bugzilla bug 1992596, which is valid. The bug has been updated to refer to the pull request using the external bug tracker.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.9.0) matches configured target release for branch (4.9.0)
bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

Requesting review from QA contact:
/cc @zhouying7780

In response to this:

WIP: Bug 1992596: e2e/cli: Move basicresources.sh to proper e2e

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.

deejross · 2021-08-20T14:58:47Z

/assign @atiratree

deejross · 2021-08-26T01:44:13Z

/retest-required

atiratree · 2021-08-26T19:30:30Z

test/extended/cli/annotation.go

+
+	var (
+		ctx = context.Background()
+		oc  = exutil.NewCLI("oc-annotation").AsAdmin()


I think we probably don't need an admin privileges here. Can you check the other tests as well?

This was required for getting node information

yup, keep it in places where you need it

atiratree · 2021-08-26T19:36:11Z

test/extended/cli/resources.go

+	return k8simage.GetE2EImage(k8simage.BusyBox)
+}
+
+func getNginxImage() string {


Is it necessary to wrap these?

Was trying to avoid an additional import in the compat.go file, but if that's not a concern I can remove these.

IMO that is not a big concern

atiratree · 2021-08-26T19:36:32Z

test/extended/cli/resources.go

+	return k8simage.GetE2EImage(k8simage.Nginx)
+}
+
+func newCreateOptions() metav1.CreateOptions {


this seems too superfluous as well

atiratree · 2021-08-26T19:49:23Z

test/extended/cli/compat.go

+		o.Expect(err).NotTo(o.HaveOccurred())
+	})
+
+	g.It("can ensure the probe command is functioning as expected on pods", func() {


a lot of these seem quite specific and they could look better in separate files. Can you move these usecases to a separate PR as this one got quite big already? It would be easier to review/merge.

atiratree · 2021-08-26T19:54:26Z

test/extended/cli/compat.go

+		o.Expect(err).NotTo(o.HaveOccurred())
+	})
+
+	g.It("can create deploymentconfig and clusterquota", func() {


we could have a special test for cluster quota that also verifies the quotas

atiratree · 2021-08-26T19:58:40Z

test/extended/cli/compat.go

+		o.Expect(out).To(o.ContainSubstring("updated"))
+	})
+
+	g.It("can ensure the expose command is functioning as expected", func() {


this could be under services or expose file

atiratree · 2021-08-26T20:07:32Z

test/extended/cli/resources.go

+
+func newMixedAPIVersionsFile() string {
+	// this was converted from YAML to JSON to prevent indentation formatting issues
+	var body = `


what is the benefit of this? Keeping these in files has a better readability IMO.

Since most of the tests are required to run in disconnected environments, using images directly in these files won't work when converted to Go. The appropriate e2e images need to be used instead, which is what the string replacement handles before writing this out to a temporary file. I didn't want to modify a file already under version control during testing.

you can do a similar thing with files to what you do in your function. 1. load file 2. replace images. 3 save to tmp file. Then we can version control them directly as yaml files.

I also noticed some of the yamls don't include images so you can use them directly as files

atiratree

@deejross Can you please cleanup the commits a bit so there is not the whole history of changes but only the commits specific to this PR?

atiratree · 2021-08-27T08:25:27Z

test/extended/cli/rsync.go

 				g.By("Verifying original file is in the local directory")
 				o.Expect(foundOriginal).To(o.BeTrue())

 				g.By("Verifying renamed file is not in the local directory")
 				o.Expect(foundModified).To(o.BeFalse())

 				g.By("Getting an error if copying to a destination directory where there is no write permission")
-				result, err = oc.Run("rsync").Args(
+				_, err = oc.Run("rsync").Args(


Execute can be used here instead

This still holds.

atiratree · 2021-08-27T08:39:39Z

test/extended/cli/resources.go

+							Name:    "testapp",
+							Image:   k8simage.GetE2EImage(k8simage.BusyBox),
+							Command: []string{"/bin/sleep"},
+							Args:    []string{"300"},


this might be a small timeout for some tests, maybe better to use httpd which does not timeout?

or some kind of nginx

atiratree · 2021-08-27T08:45:59Z

test/extended/cli/statefulset.go

+
+	g.It("creates and deletes statefulsets", func() {
+		g.By("creating a new service for the statefulset")
+		_, err := oc.KubeClient().CoreV1().Services(oc.Namespace()).Create(


I think it is better to use oc CLI client since it is the purpose of these tests

Yes please.

atiratree · 2021-08-27T08:50:37Z

test/extended/cli/statefulset.go

+
+	var (
+		ctx = context.Background()
+		oc  = exutil.NewCLI("oc-statefulset").AsAdmin()


I think we should not require an admin here

atiratree · 2021-08-27T09:02:17Z

test/extended/cli/secret.go

+	var (
+		oc              = exutil.NewCLI("oc-secret").AsAdmin()
+		testData        = exutil.FixturePath("testdata", "cmd", "test", "cmd", "testdata")
+		resourceBuilder = filepath.Join(testData, "resource-builder")


it is not necessary to have testData variable if it is not reused IMO. Also resourceBuilder needs a better naming like resourceBuilderDir or path so its usage is more apparent

atiratree · 2021-08-27T09:02:23Z

test/extended/cli/secret.go

+	defer g.GinkgoRecover()
+
+	var (
+		oc              = exutil.NewCLI("oc-secret").AsAdmin()


atiratree · 2021-08-27T09:04:25Z

test/extended/cli/secret.go

+		o.Expect(err).NotTo(o.HaveOccurred())
+
+		g.By("getting secrets without extensions")
+		_, err = oc.Run("get").Args("secret", "json-no-extension", "yml-no-extension").Output()


you can use Execute in these

atiratree · 2021-08-27T09:06:35Z

test/extended/cli/service.go

+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+)
+
+var _ = g.Describe("[sig-cli] oc service", func() {


mentioned remarks apply to this file as well

atiratree · 2021-08-27T09:07:29Z

test/extended/testdata/cmd/test/cmd/basicresources.sh

@@ -1,327 +0,0 @@
-#!/bin/bash


please only remove the lines and files that apply to this PR

openshift-merge-robot · 2021-09-06T07:21:04Z

/bugzilla refresh

The requirements for Bugzilla bugs have changed, recalculating validity.

openshift-ci · 2021-09-06T07:22:09Z

@openshift-merge-robot: This pull request references Bugzilla bug 1992596, which is invalid:

expected the bug to target the "4.10.0" release, but it targets "4.9.0" instead

Comment /bugzilla refresh to re-evaluate validity if changes to the Bugzilla bug are made, or edit the title of this pull request to link to a different bug.

In response to this:

/bugzilla refresh

The requirements for Bugzilla bugs have changed, recalculating validity.

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.

deejross · 2021-09-07T13:24:13Z

/bugzilla refresh

openshift-ci · 2021-09-07T13:24:18Z

@deejross: This pull request references Bugzilla bug 1992596, which is valid.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.10.0) matches configured target release for branch (4.10.0)
bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

Requesting review from QA contact:
/cc @zhouying7780

In response to this:

/bugzilla refresh

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.

openshift-ci · 2021-09-07T15:47:48Z

@deejross: This pull request references Bugzilla bug 1992596, which is valid.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.10.0) matches configured target release for branch (4.10.0)
bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

Requesting review from QA contact:
/cc @zhouying7780

In response to this:

Bug 1992596: e2e/cli: Move annotations.sh and basicresources.sh to proper e2e

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.

soltysh

A couple of nits, but most importantly you forgot to run make update to regenerate annotations.

soltysh · 2021-09-17T14:27:02Z

test/extended/cli/statefulset.go

+	defer g.GinkgoRecover()
+
+	var (
+		oc = exutil.NewCLI("oc-statefulset")


Nit: for single vars it's fine to just do

oc := exutil.NewCLI("oc-statefulset")

soltysh · 2021-09-17T14:27:26Z

test/extended/cli/status.go

+	defer g.GinkgoRecover()
+
+	var (
+		oc = exutil.NewCLI("oc-status")


Same here wrt oc := exutil.NewCLI("oc-status")

atiratree

@deejross can you please only remove the relevant lines as mentioned?(#26415 (comment)). This will help us to continue atomically in the next PR

atiratree · 2021-09-20T12:47:30Z

test/extended/cli/statefulset.go

+		o.Expect(err).NotTo(o.HaveOccurred())
+		defer os.Remove(statefulSetFile)
+
+		err = oc.Run("create").Args("-f", frontendFile).Execute()


btw you can also use InputString to read the data from stdin instead of a file

also can you please use the oc CLI client instead of an direct API calls in other places as well?

those can be done in followups

…oper e2e

soltysh

/lgtm

openshift-ci · 2021-09-21T16:55:05Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: deejross, soltysh

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:

~~test/extended/OWNERS~~ [soltysh]

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

openshift-ci · 2021-09-21T18:10:28Z

@deejross: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/e2e-agnostic-cmd	`4daa813`	link	false	`/test e2e-agnostic-cmd`
ci/prow/e2e-aws-csi	`4daa813`	link	false	`/test e2e-aws-csi`
ci/prow/e2e-aws-single-node	`4daa813`	link	false	`/test e2e-aws-single-node`

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

openshift-bot · 2021-09-21T18:19:44Z

/retest-required

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2021-09-21T18:32:42Z

/retest-required

Please review the full test history for this PR and help us cut down flakes.

openshift-ci · 2021-09-21T19:53:11Z

@deejross: All pull requests linked via external trackers have merged:

Bugzilla bug 1992596 has been moved to the MODIFIED state.

In response to this:

Bug 1992596: e2e/cli: Move annotations.sh and basicresources.sh to proper e2e

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.

hardys · 2022-01-11T11:37:31Z

/cherry-pick release-4.9

hardys · 2022-01-11T12:04:28Z

/cherry-pick release-4.9

openshift-cherrypick-robot · 2022-01-11T12:05:09Z

@hardys: new pull request created: #26738

In response to this:

/cherry-pick release-4.9

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.

hardys · 2022-01-11T12:05:21Z

Backporting this should resolve https://bugzilla.redhat.com/show_bug.cgi?id=1998963 I think since it removes the hard-coded busybox image reference

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 20, 2021

openshift-ci bot added bugzilla/severity-medium Referenced Bugzilla bug's severity is medium for the branch this PR is targeting. bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. labels Aug 20, 2021

openshift-ci bot requested review from zhouying7780, gabemontero and ironcladlou August 20, 2021 14:56

openshift-ci bot assigned atiratree Aug 20, 2021

deejross mentioned this pull request Aug 20, 2021

Bug 1992596: e2e/cli: Move annotations.sh to proper e2e #26410

Closed

deejross changed the title ~~WIP: Bug 1992596: e2e/cli: Move basicresources.sh to proper e2e~~ WIP: Bug 1992596: e2e/cli: Move annotations.sh and basicresources.sh to proper e2e Aug 21, 2021

atiratree reviewed Aug 26, 2021

View reviewed changes

atiratree reviewed Aug 27, 2021

View reviewed changes

openshift-ci bot mentioned this pull request Aug 31, 2021

Bug 1992596: cli: use ubi8/ruby-27 #26416

Merged

deejross changed the title ~~WIP: Bug 1992596: e2e/cli: Move annotations.sh and basicresources.sh to proper e2e~~ Bug 1992596: e2e/cli: Move annotations.sh and basicresources.sh to proper e2e Aug 31, 2021

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 31, 2021

openshift-ci bot added bugzilla/invalid-bug Indicates that a referenced Bugzilla bug is invalid for the branch this PR is targeting. and removed bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. labels Sep 6, 2021

openshift-ci bot added the bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. label Sep 7, 2021

openshift-ci bot removed the bugzilla/invalid-bug Indicates that a referenced Bugzilla bug is invalid for the branch this PR is targeting. label Sep 7, 2021

deejross force-pushed the e2e-basicresources.sh branch from fbd9a0e to 7a95488 Compare September 7, 2021 15:45

deejross force-pushed the e2e-basicresources.sh branch 2 times, most recently from f572091 to 6b4685e Compare September 7, 2021 15:55

openshift-ci bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 7, 2021

openshift-ci bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 13, 2021

soltysh approved these changes Sep 17, 2021

View reviewed changes

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 17, 2021

atiratree reviewed Sep 20, 2021

View reviewed changes

Bug 1992596: e2e/cli: Move annotations.sh and basicresources.sh to pr…

4daa813

…oper e2e

deejross force-pushed the e2e-basicresources.sh branch from 4f3c255 to 4daa813 Compare September 21, 2021 16:23

soltysh approved these changes Sep 21, 2021

View reviewed changes

openshift-ci bot assigned soltysh Sep 21, 2021

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Sep 21, 2021

openshift-merge-robot merged commit c93debe into openshift:master Sep 21, 2021

openshift-ci bot mentioned this pull request Oct 6, 2021

Bug 1992596: e2e/cli: More basicresources.sh #26502

Merged

This was referenced Nov 5, 2021

Bug 1992596: e2e/cli: Add probe tests #26570

Merged

Bug 1992596: e2e/cli: Add env tests #26573

Merged

Bug 1992596: e2e/cli: Finish basicresources.sh migration #26578

Merged

This was referenced Nov 19, 2021

Bug 1992596: e2e/cli: Finish basicresources.sh migration #26620

Merged

Bug 1992596: e2e/cli: Migrate apiresources.go and add version #26589

Merged

openshift-ci bot mentioned this pull request Jan 5, 2022

Bug 1992596: Remove admin bashcomp func openshift/oc#1014

Merged

hardys mentioned this pull request Jan 11, 2022

Move debug.sh & timeout.sh to a proper e2e #25638

Merged

openshift-cherrypick-robot mentioned this pull request Jan 11, 2022

[release-4.9] Bug 2039275: e2e/cli: Move annotations.sh and basicresources.sh to proper e2e #26738

Closed

openshift-ci bot mentioned this pull request Apr 28, 2022

Bug 1992596: e2e/cli: Migrate create.sh to Go tests #27049

Merged

openshift-ci bot mentioned this pull request May 31, 2022

Bug 1992596: move help.sh to golang-based e2e #27190

Merged

Navigation Menu

Bug 1992596: e2e/cli: Move annotations.sh and basicresources.sh to proper e2e #26415

Bug 1992596: e2e/cli: Move annotations.sh and basicresources.sh to proper e2e #26415

Conversation

deejross commented Aug 20, 2021 • edited

openshift-ci bot commented Aug 20, 2021

deejross commented Aug 20, 2021

deejross commented Aug 26, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

atiratree left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openshift-merge-robot commented Sep 6, 2021

openshift-ci bot commented Sep 6, 2021

deejross commented Sep 7, 2021

openshift-ci bot commented Sep 7, 2021

openshift-ci bot commented Sep 7, 2021

soltysh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

atiratree left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

soltysh left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Sep 21, 2021

openshift-ci bot commented Sep 21, 2021 • edited

openshift-bot commented Sep 21, 2021

openshift-bot commented Sep 21, 2021

openshift-ci bot commented Sep 21, 2021

hardys commented Jan 11, 2022

hardys commented Jan 11, 2022

openshift-cherrypick-robot commented Jan 11, 2022

hardys commented Jan 11, 2022

deejross commented Aug 20, 2021 •

edited

openshift-ci bot commented Sep 21, 2021 •

edited