USHIFT-348: microshift: skip and fix sig-cli tests #27389

pacevedom · 2022-09-01T08:56:08Z

No description provided.

dhellmann · 2022-09-01T20:39:04Z

test/extended/cli/apiresources.go

+		o.Expect(out).To(o.ContainSubstring("storage.k8s.io/v1"))
+	})
+
+	g.It("can output expected information about openshift api-resources [apigroup:build.openshift.io][apigroup:project.openshift.io][apigroup:user.openshift.io]", func() {


I'm not sure about this part of the change. Logically it's saying that if the API exists then it should show up in the output from api-resources, but aren't we using the same query to see if it exists that oc uses to produce its output? Is that a useful test?

I know we've said we didn't want to generally, but this feels like a case where we should say "If the cluster is MicroShift, skip" so that we can have a separate test that checks for the expected APIs when the cluster is MicroShift.

dhellmann · 2022-09-01T20:47:51Z

test/extended/cli/explain.go

 		{Group: "template.openshift.io", Version: "v1", Resource: "templateinstances"},
 	}

+	microShiftBuiltinTypes = []schema.GroupVersionResource{


How hard would it be for us to put these microshift-specific tests in the microshift repo? If not now, eventually?

I know @bparees has been looking at splitting up the tests, so maybe we can be an early adopter of that work?

Have local e2e tests? I have seen other operators do it before.

pacevedom · 2022-09-13T05:57:05Z

/hold
Need to rework.

pacevedom · 2022-09-14T09:36:32Z

/unhold
Ready for review. Depends on #27345
/assign @ingvagabund

ingvagabund · 2022-09-21T11:37:19Z

test/extended/cli/apiresources.go

+		o.Expect(out).To(o.ContainSubstring("storage.k8s.io/v1"))
+	})
+
+	g.It("can output expected information about openshift api-resources [apigroup:build.openshift.io][apigroup:project.openshift.io][apigroup:user.openshift.io]", func() {


To increase the flexibility it's worth introducing new g.It for each apigroup. E.g.:

g.It("can output expected information about openshift build api-resources [apigroup:build.openshift.io]", func() { ... } g.It("can output expected information about openshift image api-resources [apigroup:image.openshift.io]", func() { ... } g.It("can output expected information about openshift project api-resources [apigroup:project.openshift.io]", func() { ... } g.It("can output expected information about openshift user api-resources [apigroup:user.openshift.io]", func() { ... } ...

ingvagabund · 2022-09-21T11:41:17Z

test/extended/cli/observe.go

 		o.Expect(os.Unsetenv("MYENV")).NotTo(o.HaveOccurred())
 	})
+
+	g.It("works as expected with cluster operators [apigroup:config.openshift.io]", func() {


Why separate when the original works as expected [apigroup:config.openshift.io] name still keeps the apigroup?

ingvagabund · 2022-10-11T09:59:06Z

test/extended/cli/explain.go

+		})
+	}
+
+	for group, bts := range builtinTypes {


Is the loop duplicated by accident or is there a reason for it?

builtinTypes is defined as map[string][]schema.GroupVersionResource. This loop takes the apigroup as group and the list is assigned to bts. This is passed to verifySpecStatusExplain which takes a schema.GroupVersionResource as the last argument.

Both

for group, bts := range builtinTypes { g.It(fmt.Sprintf("should contain spec+status for %s [apigroup:%s]", group, group), func() { for _, bt := range bts { o.Expect(verifySpecStatusExplain(oc, nil, bt)).NotTo(o.HaveOccurred()) } }) }

and

for group, bts := range builtinTypes { g.It(fmt.Sprintf("should contain spec+status for %s [apigroup:%s]", group, group), func() { for _, bt := range bts { e2e.Logf("Checking %s...", bt) o.Expect(verifySpecStatusExplain(oc, nil, bt)).NotTo(o.HaveOccurred()) } }) }

are identical up to `e2e.Logf("Checking %s...", bt) line unless I am overlooking something.

Oh wait, misunderstood you. You are absolutely right, should be fixed now.

ingvagabund · 2022-10-11T10:10:09Z

test/extended/cli/explain.go

 	}
+)

+func getCrdTypes(oc *exutil.CLI) []schema.GroupVersionResource {


getCrdTypes is invoked by two tests:

"list uncovered GroupVersionResources"

"should contain proper spec+status for CRDs [apigroup:config.openshift.io]"

The second already assumes existence of the config apigroup which is getting detected inside getCrdTypes.

getCrdTypes is always constructing the list of crd types from baseCRDTypes, autoscalingTypes, machineTypes and additionalOperatorTypes which belong to their corresponding groups as well. Which at least requires to label both tests with the corresponding groups as well. Making both tests as another place for refactoring and looping through through the apigroups.

@pacevedom this tests requires more refactoring than anticipated. Though, better to do it properly now than later/never. Nevertheless, very good progress here.

"should contain proper spec+status for CRDs [apigroup:config.openshift.io]" requires config API because it checks for control plane topology in order to select different CRDs.
The nature of this test is to check explicitly for presence of those APIs that get built in getCRDTypes. If they are not present (or are poorly defined) then that should be an error. Maybe we should have a MicroShift specific test with the CRDs we install there instead? To me it is not only about checking that whatever is present is ok, it is also about specifying what should be installed.

"list uncovered GroupVersionResources" goes after API groups not covered in this file to list them. It does not perform any checks besides producing logs for those uncovered.

should contain proper spec+status for CRDs [apigroup:config.openshift.io] checks presence of explanation text for corresponding CRD's spec and status.

The nature of this test is to check explicitly for presence of those APIs that get built in getCRDTypes. If they are not present (or are poorly defined) then that should be an error.

The test does not check whether a cluster has a required list of CRDs present. Unless I am missing the specific line. The test checks whether CRDs listed by getCRDTypes have the spec and status properly described when running oc explain command. I.e. testing implication (is a given crd present -> has the crd spec/status properly explained). For that the list of CRDs can still be broken down based on the apigroup and each apigroup test separately. If there's a need to explicitly check for a presence of a specific list of CRDs, new test needs to be written.

To me it is not only about checking that whatever is present is ok, it is also about specifying what should be installed.

I don't think this is the case here.

"list uncovered GroupVersionResources" goes after API groups not covered in this file to list them. It does not perform any checks besides producing logs for those uncovered.

Ack. Reading the code a second time resourceMap gets populated from what's installed on a cluster minutes what's provided by getCrdTypes. Taking my comment back. Thanks for the pointers.

test/extended/cli/basics.go

ingvagabund · 2022-10-11T16:02:31Z

test/extended/cli/basics.go

 				TLSClientConfig: &tls.Config{
-					RootCAs: rootCAs,
+					RootCAs:      rootCAs,
+					Certificates: certs,


@pacevedom does this change automatically implies OAuth tokens will no longer be used? Is there any way to detect whether OAuth tokens are used and generate list of certs only when they are not?

Thanks for having a look, this was a leftover from before all the changes to the oc project impersionation were complete. Now its not needed so I remove it.

pacevedom · 2022-10-12T07:22:17Z

/retest-required

pmtk · 2022-10-19T09:36:03Z

/retest-required

ggiguash · 2022-10-24T09:45:50Z

/retest-required

ingvagabund · 2022-10-26T13:17:54Z

/lgtm

mfojtik · 2022-10-26T13:21:56Z

/lgtm
/approve

openshift-ci · 2022-10-26T13:25:17Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ingvagabund, mfojtik, pacevedom

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~test/OWNERS~~ [mfojtik]

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

pmtk · 2022-10-26T16:16:37Z

/retest-required

openshift-ci · 2022-10-26T19:52:46Z

@pacevedom: 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-openstack-ovn	`baf4c04`	link	false	`/test e2e-openstack-ovn`
ci/prow/e2e-gcp-ovn-rt-upgrade	`baf4c04`	link	false	`/test e2e-gcp-ovn-rt-upgrade`
ci/prow/e2e-agnostic-ovn-cmd	`baf4c04`	link	false	`/test e2e-agnostic-ovn-cmd`
ci/prow/e2e-aws-ovn-serial	`baf4c04`	link	true	`/test e2e-aws-ovn-serial`
ci/prow/e2e-metal-ipi-ovn-ipv6	`baf4c04`	link	false	`/test e2e-metal-ipi-ovn-ipv6`
ci/prow/e2e-aws-ovn-single-node-upgrade	`baf4c04`	link	false	`/test e2e-aws-ovn-single-node-upgrade`
ci/prow/e2e-gcp-csi	`baf4c04`	link	false	`/test e2e-gcp-csi`
ci/prow/e2e-metal-ipi-sdn	`baf4c04`	link	false	`/test e2e-metal-ipi-sdn`
ci/prow/e2e-aws-ovn-single-node-serial	`baf4c04`	link	false	`/test e2e-aws-ovn-single-node-serial`
ci/prow/e2e-aws-ovn-cgroupsv2	`baf4c04`	link	false	`/test e2e-aws-ovn-cgroupsv2`
ci/prow/e2e-aws-csi	`baf4c04`	link	false	`/test e2e-aws-csi`
ci/prow/e2e-aws-ovn-single-node	`baf4c04`	link	false	`/test e2e-aws-ovn-single-node`
ci/prow/e2e-gcp-ovn	`baf4c04`	link	true	`/test e2e-gcp-ovn`
ci/prow/e2e-gcp-ovn-upgrade	`baf4c04`	link	true	`/test e2e-gcp-ovn-upgrade`

Full PR test history. Your PR dashboard.

Details

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.

ingvagabund · 2022-10-27T14:57:19Z

Merged through #27498
/close

openshift-ci · 2022-10-27T15:03:02Z

@ingvagabund: Closed this PR.

Details

In response to this:

Merged through #27498
/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/test-infra repository.

openshift-ci bot requested review from deads2k and sosiouxme September 1, 2022 08:57

dhellmann reviewed Sep 1, 2022

View reviewed changes

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 8, 2022

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

pacevedom force-pushed the ushift-sig-cli branch from 8cd15c6 to 5d90fed Compare September 14, 2022 09:36

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 14, 2022

openshift-ci bot assigned ingvagabund Sep 14, 2022

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

pacevedom force-pushed the ushift-sig-cli branch from 5d90fed to 762f389 Compare September 14, 2022 10:07

ingvagabund reviewed Sep 21, 2022

View reviewed changes

pacevedom force-pushed the ushift-sig-cli branch from 762f389 to 4106c86 Compare October 10, 2022 21:38

ingvagabund suggested changes Oct 11, 2022

View reviewed changes

ingvagabund reviewed Oct 11, 2022

View reviewed changes

test/extended/cli/basics.go Outdated Show resolved Hide resolved

pacevedom force-pushed the ushift-sig-cli branch 2 times, most recently from 0730bb9 to e47e136 Compare October 11, 2022 14:56

ingvagabund reviewed Oct 11, 2022

View reviewed changes

pacevedom force-pushed the ushift-sig-cli branch from e47e136 to d111353 Compare October 11, 2022 21:34

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 24, 2022

microshift: skip and fix sig-cli tests

baf4c04

pacevedom force-pushed the ushift-sig-cli branch from d111353 to baf4c04 Compare October 26, 2022 12:28

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 26, 2022

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

openshift-ci bot assigned mfojtik Oct 26, 2022

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

ingvagabund mentioned this pull request Oct 26, 2022

MicroShift: label e2es with more apigroup labels (squashing more PRs into a single one + fixes) #27498

Merged

openshift-ci bot closed this Oct 27, 2022

USHIFT-348: microshift: skip and fix sig-cli tests #27389

USHIFT-348: microshift: skip and fix sig-cli tests #27389

Uh oh!

Conversation

pacevedom commented Sep 1, 2022

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!

pacevedom commented Sep 13, 2022

Uh oh!

pacevedom commented Sep 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ingvagabund Sep 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

ingvagabund Oct 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

ingvagabund Oct 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pacevedom commented Oct 12, 2022

Uh oh!

pmtk commented Oct 19, 2022

Uh oh!

ggiguash commented Oct 24, 2022

Uh oh!

ingvagabund commented Oct 26, 2022

Uh oh!

mfojtik commented Oct 26, 2022

Uh oh!

openshift-ci bot commented Oct 26, 2022

Uh oh!

pmtk commented Oct 26, 2022

Uh oh!

openshift-ci bot commented Oct 26, 2022

Uh oh!

ingvagabund commented Oct 27, 2022

Uh oh!

openshift-ci bot commented Oct 27, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

pacevedom commented Sep 14, 2022 •

edited

Loading

ingvagabund Sep 21, 2022 •

edited

Loading

ingvagabund Oct 11, 2022 •

edited

Loading

ingvagabund Oct 12, 2022 •

edited

Loading