Bug 1827603: vSphere - Unit tests for zone and regions #569

alexander-demicev · 2020-04-23T18:11:13Z

This PR adds unit tests for zone and regions in vSphere. Regions and zones are not always present on the machine, this depends on vSphere configuration but we can lower risks of an error on our side and make sure we set them properly.

elmiko · 2020-04-23T19:01:53Z

pkg/controller/vsphere/util_test.go

+
+func TestGetVSphereConfig(t *testing.T) {
+	testConfig := `
+    [Labels]


is this a leftover bit or something? i don't recognize the syntax here.

That's INI config, that comes from cloud cloud-provider-config config map. Had to learn that too :)

oh, interesting, guess i need to learn it as well XD

Who chose INI XD

elmiko · 2020-04-23T19:57:45Z

i will defer to others on the vsphere specifics, but in general this looks good to me

openshift-ci-robot · 2020-04-24T10:07:11Z

@alexander-demichev: This pull request references Bugzilla bug 1827603, which is valid. The bug has been moved to the POST state. 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.5.0) matches configured target release for branch (4.5.0)
bug is in the state NEW, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

In response to this:

Bug 1827603: vSphere - Unit tests for zone and regions

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.

alexander-demicev · 2020-04-24T10:07:12Z

/retest

JoelSpeed · 2020-04-24T10:36:33Z

pkg/controller/vsphere/reconciler_test.go

+	createAndAttachTag := func(tagName, categoryName string) {
+		if err := session.WithRestClient(context.TODO(), func(c *rest.Client) error {
+			tagsMgr := tags.NewManager(c)
+
+			categoryID, err := tagsMgr.CreateCategory(context.TODO(), &tags.Category{
+				AssociableTypes: []string{"VirtualMachine"},
+				Cardinality:     "SINGLE",
+				Name:            categoryName,
+			})
+			if err != nil {
+				return err
+			}
+
+			tagID, err := tagsMgr.CreateTag(context.TODO(), &tags.Tag{
+				CategoryID: categoryID,
+				Name:       tagName,
+			})
+			if err != nil {
+				return err
+			}
+
+			err = tagsMgr.AttachTag(context.TODO(), tagID, vmObj.Reference())
+			if err != nil {
+				return err
+			}
+
+			return nil
+		}); err != nil {
+			t.Fatal(err)
+		}
+	}


I feel like I've seen this before in another PR, could this be a proper function rather than an anonymous one that multiple tests could use?

JoelSpeed · 2020-04-24T10:38:35Z

pkg/controller/vsphere/reconciler_test.go

+	}
+
+	if regionName != labels[machinecontroller.MachineRegionLabelName] {
+		t.Errorf("Expected region name: %s, got: %s", zoneName, labels[machinecontroller.MachineRegionLabelName])


Typo?

Suggested change

t.Errorf("Expected region name: %s, got: %s", zoneName, labels[machinecontroller.MachineRegionLabelName])

t.Errorf("Expected region name: %s, got: %s", regionName, labels[machinecontroller.MachineRegionLabelName])

JoelSpeed · 2020-04-24T10:39:25Z

pkg/controller/vsphere/util_test.go

+
+func TestGetVSphereConfig(t *testing.T) {
+	testConfig := `
+    [Labels]


Who chose INI XD

JoelSpeed · 2020-04-24T10:40:55Z

pkg/controller/vsphere/util_test.go

+	if vSphereConfig.Labels.Region != "testRegion" {
+		t.Errorf("Expected region %s, got %s", "testRegion", vSphereConfig.Labels.Region)
+	}
+
+	if vSphereConfig.Labels.Zone != "testZone" {
+		t.Errorf("Expected zone %s, got %s", "testZone", vSphereConfig.Labels.Zone)
+	}


Maybe put testZone and testRegion into consts?

enxebre · 2020-04-29T09:56:16Z

thanks for this!
/approve
/retest

openshift-ci-robot · 2020-04-29T09:56:33Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: enxebre

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

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

JoelSpeed

/lgtm

JoelSpeed · 2020-04-29T12:53:25Z

pkg/controller/vsphere/reconciler_test.go

+	}
+}
+
+func createTagAndCategory(session *session.Session, categoryName, tagName string) error {


Thanks for doing this 👍

openshift-bot · 2020-04-29T13:11:46Z

/retest

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

openshift-bot · 2020-04-29T14:55:22Z

/retest

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

openshift-bot · 2020-04-29T15:08:20Z

/retest

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

openshift-bot · 2020-04-29T15:21:17Z

/retest

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

openshift-bot · 2020-04-29T15:34:18Z

/retest

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

openshift-bot · 2020-04-29T15:47:19Z

/retest

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

openshift-bot · 2020-04-29T16:00:20Z

/retest

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

openshift-bot · 2020-04-29T16:13:20Z

/retest

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

openshift-bot · 2020-04-29T16:26:44Z

/retest

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

openshift-bot · 2020-04-29T16:52:24Z

/retest

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

openshift-bot · 2020-04-29T17:18:23Z

/retest

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

openshift-bot · 2020-04-30T06:30:44Z

/retest

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

openshift-bot · 2020-04-30T06:43:47Z

/retest

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

openshift-bot · 2020-04-30T06:56:44Z

/retest

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

openshift-bot · 2020-04-30T07:09:42Z

/retest

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

openshift-bot · 2020-04-30T07:22:50Z

/retest

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

openshift-bot · 2020-04-30T07:35:55Z

/retest

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

enxebre · 2020-04-30T07:36:23Z

/hold
bot is spamming with failing tests, please rebase.

JoelSpeed

/lgtm

alexander-demicev · 2020-04-30T10:34:54Z

/hold cancel

openshift-bot · 2020-04-30T10:50:46Z

/retest

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

openshift-bot · 2020-04-30T11:03:52Z

/retest

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

openshift-bot · 2020-04-30T11:29:53Z

/retest

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

openshift-bot · 2020-04-30T12:21:46Z

/retest

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

openshift-bot · 2020-04-30T13:13:53Z

/retest

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

openshift-bot · 2020-04-30T13:26:45Z

/retest

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

openshift-bot · 2020-04-30T14:05:47Z

/retest

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

openshift-bot · 2020-04-30T15:10:32Z

/retest

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

openshift-bot · 2020-04-30T15:24:04Z

/retest

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

openshift-ci-robot · 2020-04-30T15:31:27Z

@alexander-demichev: The following tests failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
ci/prow/e2e-azure	`8e1a42a`	link	`/test e2e-azure`
ci/prow/e2e-gcp	`8e1a42a`	link	`/test e2e-gcp`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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 · 2020-04-30T15:49:36Z

/retest

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

openshift-ci-robot · 2020-04-30T16:20:19Z

@alexander-demichev: All pull requests linked via external trackers have merged: openshift/machine-api-operator#569. Bugzilla bug 1827603 has been moved to the MODIFIED state.

In response to this:

Bug 1827603: vSphere - Unit tests for zone and regions

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-robot requested review from elmiko and JoelSpeed April 23, 2020 18:11

elmiko reviewed Apr 23, 2020

View reviewed changes

alexander-demicev changed the title ~~vSphere - Unit tests for zone and regions~~ Bug 1827603: vSphere - Unit tests for zone and regions Apr 24, 2020

openshift-ci-robot added bugzilla/unspecified bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. labels Apr 24, 2020

JoelSpeed reviewed Apr 24, 2020

View reviewed changes

alexander-demicev force-pushed the unittestregions branch from 7a47bd0 to 188911d Compare April 27, 2020 12:16

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 29, 2020

JoelSpeed reviewed Apr 29, 2020

View reviewed changes

openshift-ci-robot assigned JoelSpeed Apr 29, 2020

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 29, 2020

openshift-ci-robot added bugzilla/severity-unspecified Referenced Bugzilla bug's severity is unspecified for the PR. and removed bugzilla/unspecified labels Apr 29, 2020

openshift-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Apr 30, 2020

Test zone and regions for vsphere

8e1a42a

alexander-demicev force-pushed the unittestregions branch from 188911d to 8e1a42a Compare April 30, 2020 09:41

openshift-ci-robot removed lgtm Indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Apr 30, 2020

JoelSpeed reviewed Apr 30, 2020

View reviewed changes

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 30, 2020

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

openshift-merge-robot merged commit 008d912 into openshift:master Apr 30, 2020

	t.Errorf("Expected region name: %s, got: %s", zoneName, labels[machinecontroller.MachineRegionLabelName])
	t.Errorf("Expected region name: %s, got: %s", regionName, labels[machinecontroller.MachineRegionLabelName])

Bug 1827603: vSphere - Unit tests for zone and regions #569

Bug 1827603: vSphere - Unit tests for zone and regions #569

Conversation

alexander-demicev commented Apr 23, 2020 • edited

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

elmiko commented Apr 23, 2020

openshift-ci-robot commented Apr 24, 2020

alexander-demicev commented Apr 24, 2020

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

enxebre commented Apr 29, 2020

openshift-ci-robot commented Apr 29, 2020

JoelSpeed left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openshift-bot commented Apr 29, 2020

openshift-bot commented Apr 29, 2020

openshift-bot commented Apr 29, 2020

openshift-bot commented Apr 29, 2020

openshift-bot commented Apr 29, 2020

openshift-bot commented Apr 29, 2020

openshift-bot commented Apr 29, 2020

openshift-bot commented Apr 29, 2020

openshift-bot commented Apr 29, 2020

openshift-bot commented Apr 29, 2020

openshift-bot commented Apr 29, 2020

openshift-bot commented Apr 30, 2020

openshift-bot commented Apr 30, 2020

openshift-bot commented Apr 30, 2020

openshift-bot commented Apr 30, 2020

openshift-bot commented Apr 30, 2020

openshift-bot commented Apr 30, 2020

enxebre commented Apr 30, 2020

JoelSpeed left a comment

Choose a reason for hiding this comment

alexander-demicev commented Apr 30, 2020

openshift-bot commented Apr 30, 2020

openshift-bot commented Apr 30, 2020

openshift-bot commented Apr 30, 2020

openshift-bot commented Apr 30, 2020

openshift-bot commented Apr 30, 2020

openshift-bot commented Apr 30, 2020

openshift-bot commented Apr 30, 2020

openshift-bot commented Apr 30, 2020

openshift-bot commented Apr 30, 2020

openshift-ci-robot commented Apr 30, 2020 • edited

openshift-bot commented Apr 30, 2020

openshift-ci-robot commented Apr 30, 2020

alexander-demicev commented Apr 23, 2020 •

edited

openshift-ci-robot commented Apr 30, 2020 •

edited