Migrate to openshift/api by alexander-demicev · Pull Request #943 · openshift/machine-api-operator

alexander-demicev · 2021-10-26T18:03:00Z

This PR migrates API types to openshift/api, that includes:

removing api package and replacing it with openshift/api imports
utils/conditions package refactor
moving methods that were defined for api types to controllers https://github.com/openshift/machine-api-operator/blob/master/pkg/apis/machine/v1beta1/machine_types.go#L198 https://github.com/openshift/machine-api-operator/blob/master/pkg/apis/machine/v1beta1/machineset_types.go#L161
removing generated client and replacing it with client-go
moving webhook related code to a separate package

elmiko

this looks generally good to me, i just have a question and a comment:

do the validateMachine and validateMachineset functions get tested in the normal webhook unit tests?

i agree with your comment from slack about removing the generate targets from the makefile too.

alexander-demicev · 2021-10-27T11:59:09Z

/retest-required

alexander-demicev · 2021-10-27T13:42:06Z

/retest-required

alexander-demicev · 2021-10-27T13:43:49Z

@elmiko I don't think these functions are tested as a part of webhooks tests, but it looks to me that these functions can be moved to webhooks entirely.

elmiko · 2021-10-27T13:51:28Z

@elmiko I don't think these functions are tested as a part of webhooks tests, but it looks to me that these functions can be moved to webhooks entirely.

whichever you think is best. if the functions are only used here, then it might make sense to just keep them in this repo, but if we do we should add unit tests for them.

alexander-demicev · 2021-10-27T13:54:10Z

@elmiko I agree but new unit tests are out of scope for this PR, lets create a card for it.

elmiko · 2021-10-27T21:43:44Z

/approve

openshift-ci · 2021-10-27T21:44:27Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: elmiko

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:

~~OWNERS~~ [elmiko]

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

JoelSpeed

I very much like the consistency that this PR will bring, left some minor stuff inline, PTAL

JoelSpeed · 2021-10-29T09:48:03Z

@@ -1,9 +0,0 @@
-FILE_DIFF=$(git ls-files -o --exclude-standard)


Is this file not used to verify that the vendored dependencies are in sync? If it isn't, it should be 🤔

It doesn't look like we use it for verifying vendored stuff, I brought this script back. Let's make a card for adding a CI job for it.

JoelSpeed · 2021-10-29T09:57:39Z

+func WrapMachineHealthCheck(mhc *machinev1.MachineHealthCheck) *MachineHealthCheckWrapper {
+	return &MachineHealthCheckWrapper{MachineHealthCheck: mhc}
+}


Is this used? We don't have one for Machine? Also, do we need MachineSet wrappers?

I will delete it. MachineSets do not use conditions in out implementation.

JoelSpeed · 2021-10-29T09:59:20Z

+	// PausedAnnotation is an annotation that can be applied to MachineHealthCheck objects to prevent the MHC controller
+	// from processing it.
+	// TODO: move this annotation to the openshift/api package
+	PausedAnnotation = "cluster.x-k8s.io/paused"


This is now duplicated with somewhere else in this PR, can we make one consistent location for it

Yes, we can, the consistent location is openshift/api. I opened a PR for it openshift/api#1046

I'll remove it from here once we merge the API PR.

I know you're keen to merge this soon, so perhaps to avoid having to wait, could you make a jira card for next sprint or a BZ to make sure we don't forget to update this, and then make the annotation variable private where it is used for now, then it should be easy to change later

JoelSpeed · 2021-10-29T11:13:08Z

/lgtm

openshift-bot · 2021-10-29T12:13:36Z

/retest-required

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

openshift-bot · 2021-10-29T12:26:37Z

/retest-required

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

openshift-bot · 2021-10-29T12:39:36Z

/retest-required

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

openshift-ci · 2021-10-29T12:43:58Z

@alexander-demichev: 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/generate	`dc97c12`	link	true	`/test generate`
ci/prow/e2e-vsphere	`ecbdcb3`	link	false	`/test e2e-vsphere`
ci/prow/e2e-vsphere-operator	`ecbdcb3`	link	false	`/test e2e-vsphere-operator`
ci/prow/e2e-gcp-operator	`ecbdcb3`	link	false	`/test e2e-gcp-operator`
ci/prow/e2e-vsphere-upgrade	`ecbdcb3`	link	false	`/test e2e-vsphere-upgrade`
ci/prow/e2e-aws-disruptive	`ecbdcb3`	link	false	`/test e2e-aws-disruptive`

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.

openshift-bot · 2021-10-29T13:04:35Z

/retest-required

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

openshift-bot · 2021-10-29T13:16:34Z

/retest-required

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

This adjusts cluster-api-provider to the changes from openshift/machine-api-operator#943

alexander-demicev added 7 commits October 26, 2021 19:52

Update dependencies

3ac4f75

Remove generated client

6602a0d

Move all webhooks related code to a separate package

6615b23

Move some validation from api to controllers

ce0b1f4

Refactor conditions package

a879579

Remove api package

d4a79a0

Replace api types imports

806785e

openshift-ci Bot requested review from SamuelStuchly and slintes October 26, 2021 18:03

elmiko reviewed Oct 26, 2021

View reviewed changes

alexander-demicev force-pushed the migratetoapi branch from ccf25f8 to dc97c12 Compare October 27, 2021 10:09

alexander-demicev mentioned this pull request Oct 27, 2021

Machine API: Remove generate check openshift/release#23054

Merged

openshift-ci Bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 27, 2021

JoelSpeed reviewed Oct 29, 2021

View reviewed changes

alexander-demicev added 3 commits October 29, 2021 12:38

Remove generation related code

752fc6d

Fix unit tests

52fedf8

Combine conditions interfaces

ecbdcb3

alexander-demicev force-pushed the migratetoapi branch from dc97c12 to ecbdcb3 Compare October 29, 2021 10:44

openshift-ci Bot assigned JoelSpeed Oct 29, 2021

openshift-ci Bot added the lgtm Indicates that a PR is ready to be merged. label Oct 29, 2021

openshift-merge-robot merged commit 128c5c9 into openshift:master Oct 29, 2021

cfergeau added a commit to cfergeau/cluster-api-provider-libvirt that referenced this pull request Jan 31, 2023

Update to newer machine/v1beta and client API

05238e7

This adjusts cluster-api-provider to the changes from openshift/machine-api-operator#943

		@@ -1,9 +0,0 @@
		FILE_DIFF=$(git ls-files -o --exclude-standard)

Conversation

alexander-demicev commented Oct 26, 2021

Uh oh!

elmiko left a comment

Choose a reason for hiding this comment

Uh oh!

alexander-demicev commented Oct 27, 2021

Uh oh!

alexander-demicev commented Oct 27, 2021

Uh oh!

alexander-demicev commented Oct 27, 2021

Uh oh!

elmiko commented Oct 27, 2021

Uh oh!

alexander-demicev commented Oct 27, 2021

Uh oh!

elmiko commented Oct 27, 2021

Uh oh!

openshift-ci Bot commented Oct 27, 2021

Uh oh!

JoelSpeed 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!

Uh oh!

Uh oh!

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!

JoelSpeed commented Oct 29, 2021

Uh oh!

openshift-bot commented Oct 29, 2021

Uh oh!

openshift-bot commented Oct 29, 2021

Uh oh!

openshift-bot commented Oct 29, 2021

Uh oh!

openshift-ci Bot commented Oct 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openshift-bot commented Oct 29, 2021

Uh oh!

openshift-bot commented Oct 29, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

openshift-ci Bot commented Oct 29, 2021 •

edited

Loading