Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Remove unnecessary machine specific client #70

Merged
merged 3 commits into from Feb 26, 2020
Merged

Remove unnecessary machine specific client #70

merged 3 commits into from Feb 26, 2020

Conversation

alexander-demicev
Copy link
Contributor

@alexander-demicev alexander-demicev commented Jan 6, 2020 •
edited

This PR removes unnecessary machine specific client, it can be replaced with controller runtime client.

@openshift-ci-robot openshift-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Jan 6, 2020
@alexander-demicev
Copy link
Contributor Author

/retest

1 similar comment
@alexander-demicev
Copy link
Contributor Author

/retest

@enxebre
Copy link
Member

enxebre commented Jan 8, 2020

can we please have a PR description with the motivation of this PR?

This PR is just removing the object at instantiation time, but it's still being passed around and relied upon.
https://github.com/openshift/cluster-api-provider-gcp/blob/master/pkg/cloud/gcp/actuators/machine/actuator.go#L35

https://github.com/openshift/cluster-api-provider-gcp/blob/master/pkg/cloud/gcp/actuators/machine/actuator.go#L43

https://github.com/openshift/cluster-api-provider-gcp/blob/master/pkg/cloud/gcp/actuators/machine/machine_scope.go#L149

@openshift-ci-robot openshift-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jan 9, 2020
@enxebre
Copy link
Member

enxebre commented Jan 14, 2020

/approve
@bison PTAL
@alexander-demichev can you please put a follow up to consolidate with this approach https://github.com/openshift/machine-api-operator/blob/master/pkg/controller/vsphere/machine_scope.go#L88 and use patch requests instead?

@openshift-ci-robot
Copy link
Contributor

[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:

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

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 14, 2020
@alexander-demicev
Copy link
Contributor Author

/retest

3 similar comments
@alexander-demicev
Copy link
Contributor Author

/retest

@alexander-demicev
Copy link
Contributor Author

/retest

@alexander-demicev
Copy link
Contributor Author

/retest

frobware
frobware reviewed Jan 20, 2020
View reviewed changes
pkg/cloud/gcp/actuators/machine/machine_scope.go Outdated
@@ -146,11 +137,12 @@ func (s *machineScope) storeMachineSpec() error {

klog.V(4).Infof("Storing machine spec for %q, resourceVersion: %v, generation: %v", s.machine.Name, s.machine.ResourceVersion, s.machine.Generation)
s.machine.Spec.ProviderSpec.Value = ext
latestMachine, err := s.machineClient.Update(s.machine)

err = s.coreClient.Update(context.Background(), s.machine)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The remainder can be reduced to just:

return s.coreClient.Update(context.Background(), s.machine)

frobware
frobware reviewed Jan 20, 2020
View reviewed changes
pkg/cloud/gcp/actuators/machine/machine_scope.go Outdated
@@ -169,11 +161,10 @@ func (s *machineScope) storeMachineStatus() error {
s.machine.Status.ProviderStatus = ext
time := metav1.Now()
s.machine.Status.LastUpdated = &time
latestMachine, err := s.machineClient.UpdateStatus(s.machine)
err = s.coreClient.Status().Update(context.Background(), s.machine)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The remainder can be reduced to just:

return s.coreClient.Status().Update(context.Background(), s.machine)

@enxebre
Copy link
Member

enxebre commented Jan 20, 2020

/retest

1 similar comment
@alexander-demicev
Copy link
Contributor Author

/retest

@alexander-demicev
Copy link
Contributor Author

/retry

@alexander-demicev
Copy link
Contributor Author

/retest

5 similar comments
@alexander-demicev
Copy link
Contributor Author

/retest

@alexander-demicev
Copy link
Contributor Author

/retest

@enxebre
Copy link
Member

enxebre commented Jan 27, 2020

/retest

@alexander-demicev
Copy link
Contributor Author

/retest

@alexander-demicev
Copy link
Contributor Author

/retest

@Danil-Grigorev Danil-Grigorev mentioned this pull request Jan 29, 2020
Closed
@alexander-demicev
Copy link
Contributor Author

/retest

2 similar comments
@alexander-demicev
Copy link
Contributor Author

/retest

@enxebre
Copy link
Member

enxebre commented Feb 4, 2020

/retest

@alexander-demicev
Copy link
Contributor Author

/test e2e-gcp-operator

@enxebre
Copy link
Member

enxebre commented Feb 11, 2020

/retest

JoelSpeed
JoelSpeed reviewed Feb 21, 2020
View reviewed changes
pkg/cloud/gcp/actuators/machine/machine_scope.go Outdated
}

func (s *machineScope) PatchMachine() error {
klog.V(3).Infof("%v: patching machine", s.machine.GetName())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: should this be a string instead?

Suggested change
klog.V(3).Infof("%v: patching machine", s.machine.GetName())
klog.V(3).Infof("%s: patching machine", s.machine.GetName())

JoelSpeed
JoelSpeed approved these changes Feb 26, 2020
View reviewed changes
Copy link
Contributor

@JoelSpeed JoelSpeed left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 26, 2020
@openshift-bot
Copy link
Contributor

/retest

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

@openshift-merge-robot openshift-merge-robot merged commit fb77f25 into openshift:master Feb 26, 2020
RadekManak pushed a commit to RadekManak/cluster-api-provider-gcp that referenced this pull request Apr 12, 2023
@k8s-ci-robot
Merge pull request openshift#70 from alexander-demicev/fixbuild
3e7fa21 
🌱 Install kustomize when releasing manifests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@frobware frobware frobware left review comments

@JoelSpeed JoelSpeed JoelSpeed approved these changes

@michaelgugino michaelgugino Awaiting requested review from michaelgugino

Assignees

@JoelSpeed JoelSpeed

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@alexander-demicev @enxebre @openshift-ci-robot @openshift-bot @frobware @JoelSpeed @openshift-merge-robot