[Digital Ocean] Code cleanup with no functional modifications #11592

srikiz · 2021-05-24T18:12:52Z

Using interfaces for better unit testing (haven't added yet, but will create that separately)
Removing cyclic dependency - moved the code from pkg/resources/digitalocean/cloud.go to upup/pkg/fi/cloudup/do/cloud.go.
No changes to the functionality, but just moving code so it's easier to maintain going forward.

johngmyers · 2021-05-24T18:20:52Z

Please edit commit history so reviewers don't have to see the back-and-forth. A series of smaller commits is better, making it easier for reviewers to verify that any given refactoring has no functional modification, but the stream should not add an unrelated file and then delete it later.

timoreimann

Looks good from a DO perspective. I'll leave the official approval to those that bear the right powers. :)

timoreimann · 2021-05-25T10:48:12Z

/unassign

johngmyers · 2021-05-25T15:07:10Z

@srikiz please edit the commit stream so it doesn't add status_discovery.go and then delete it again.

srikiz · 2021-05-25T15:59:14Z

@srikiz please edit the commit stream so it doesn't add status_discovery.go and then delete it again.

@johngmyers - is it okay if I squash all the commits once I fix couple of bazel issues?

srikiz · 2021-05-25T16:05:47Z

/hold

johngmyers · 2021-05-25T17:14:24Z

@srikiz I'm concerned this might be a bit too big to review in one go. It's easier to review if code is moved in one commit and then cleaned up in a separate commit.

srikiz · 2021-05-26T16:16:45Z

@johngmyers - that sounds like a plan. Once I get the issues resolved, I will close this pull request, and create a new one with 2 commits as you suggested. I'll just move the file in the first commit and keep the rest of the changes in a different commit.

johngmyers · 2021-05-26T16:23:53Z

@srikiz alternatively you could force-push to this PR.

…/cloudup/do directory

srikiz · 2021-05-27T21:07:39Z

@johngmyers - Hopefully, I got it right :)
In my first commit, I just moved over the file from pkg/resources/digitalocean/cloud.go to upup/pkg/cloudup/do/cloud.go - with the same content.
In my second commit, I have modified the file to make it really work.

All other changes are updating the dependencies to point to the new interface.
Hopefully this helps, please feel free to let me know if you need any additional changes to make it easier for review.

Thanks !

johngmyers

Unfortunately, the second commit is to big. I cannot easily follow to verify the lack of functional modifications. The fact that functions are being reordered at the same time as they or other things are being modified makes the diffs extremely hard to follow.

It also looks like the first commit won't compile, but is fixed up in the second commit. I can work around some of that, but it makes reviewing a harder task.

upup/pkg/fi/cloudup/do/cloud.go

johngmyers · 2021-05-28T02:18:40Z

upup/pkg/fi/cloudup/do/cloud.go.new

@@ -0,0 +1,566 @@
+/*


This file should not be added then removed. Please edit it out of the commit stream.

Sure, Removed it.

protokube/pkg/protokube/do_volume.go

srikiz · 2021-05-28T16:21:14Z

Unfortunately, the second commit is to big. I cannot easily follow to verify the lack of functional modifications. The fact that functions are being reordered at the same time as they or other things are being modified makes the diffs extremely hard to follow.

It also looks like the first commit won't compile, but is fixed up in the second commit. I can work around some of that, but it makes reviewing a harder task.

@johngmyers - Apologize, its creating a problem in reviewing this. My first commit is failing since I just moved the same file over to upup/pkg/fi/cloudup/do. But I fixed all issues in my second commit, but placing of the functions didn't match well with the original code.

johngmyers · 2021-05-28T16:43:09Z

(I haven't yet had time to review your latest push.)

@srikiz my primary task as reviewer is to verify the claim that there are no functional modifications. Like any human, I have limited short-term memory, so the fewer things I have to track at once the more practical the task.

So the request is to have a series of self-contained commits, so that I can review one commit, then discard most state about that commit and move to the next. As an exception to the expectation that each commit should be self-contained and compile, it is customary for larger changes to automatically generated files to be in a separate commit.

You might find it helpful to look at some of my refactoring PRs, for example #11219 (though that has functional changes mixed in).

…update protokube

srikiz · 2021-05-28T17:20:40Z

/retest

srikiz · 2021-05-28T17:22:43Z

@johngmyers - thank you for your time on this. My updated commit stream should hopefully make it easier now. I have re-ordered the functions so it is in-line with the changes in the first commit. Please feel free to let me know if you need further modifications. Thanks again, for the help !!

johngmyers · 2021-05-29T23:00:43Z

upup/pkg/fi/cloudup/do/cloud.go

+// DOCloud exposes all the interfaces required to operate on DigitalOcean resources
+type DOCloud interface {
+	fi.Cloud
+	//Client() *godo.Client


nit: this should be removed

Sure, updated.

johngmyers · 2021-05-29T23:02:14Z

upup/pkg/fi/cloudup/do/cloud.go

+	//Client() *godo.Client
+	DropletsService() godo.DropletsService
+	DropletActionService() godo.DropletActionsService
+	VolumeService() godo.StorageService


Since this is specific to DO, perhaps it should stick with the DO terminology.

I'll probably keep this as-is for now, and rename this to StorageService as a separate PR.

johngmyers · 2021-05-29T23:04:17Z

upup/pkg/fi/cloudup/do/cloud.go

+
+	dns dnsprovider.Interface
+
+	// region holds the DO region


nit: should be a complete sentence, including punctuation:

Suggested change

// region holds the DO region

// region holds the DO region.

Sure, updated.

johngmyers · 2021-05-29T23:06:15Z

upup/pkg/fi/cloudup/do/cloud.go

-func (c *Cloud) DNS() (dnsprovider.Interface, error) {
-	return c.dns, nil
+func (c *doCloudImplementation) DNS() (dnsprovider.Interface, error) {
+	provider := dns.NewProvider(c.Client)


Why doesn't this return c.dns?

Missed that, updated it.

johngmyers

These comments are all small enough to leave to follow-up work.

Thank you, that was much easier to review.

johngmyers · 2021-05-29T23:07:46Z

upup/pkg/fi/cloudup/do/cloud.go

+
+// FindVPCInfo is not implemented, it's only here to satisfy the fi.Cloud interface
+func (c *doCloudImplementation) FindVPCInfo(id string) (*fi.VPCInfo, error) {
+	return nil, errors.New("Not implemented")


Go errors should always start with lowercase letters

Suggested change

return nil, errors.New("Not implemented")

return nil, errors.New("not implemented")

sure, updated.

johngmyers · 2021-05-29T23:09:16Z

upup/pkg/fi/cloudup/do/mock_do_cloud.go

+// 	MockLoadBalancers godo.LoadBalancersService
+// 	MockDomain        godo.DomainsService
+// 	MockActions       godo.ActionsService
+// }


Remove commented-out code.

Sure, removed.

johngmyers · 2021-05-29T23:10:02Z

upup/pkg/fi/cloudup/do/mock_do_cloud.go

+
+// FindVPCInfo is not implemented, it's only here to satisfy the fi.Cloud interface
+func (c *doCloudMockImplementation) FindVPCInfo(id string) (*fi.VPCInfo, error) {
+	return nil, errors.New("Not implemented")


sure, updated.

johngmyers · 2021-05-29T23:10:12Z

upup/pkg/fi/cloudup/do/mock_do_cloud.go

+}
+
+func (c *doCloudMockImplementation) DeleteInstance(instance *cloudinstances.CloudInstance) error {
+	return errors.New("Not tested")


k8s-ci-robot · 2021-05-29T23:13:19Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: johngmyers, timoreimann

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

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

johngmyers · 2021-06-01T14:11:55Z

/lgtm

srikiz · 2021-06-01T14:16:28Z

/hold cancel

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels May 24, 2021

k8s-ci-robot requested review from johngmyers and zetaab May 24, 2021 18:13

k8s-ci-robot added the area/provider/digitalocean Issues or PRs related to digitalocean provider label May 24, 2021

timoreimann approved these changes May 25, 2021

View reviewed changes

k8s-ci-robot assigned timoreimann May 25, 2021

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 25, 2021

k8s-ci-robot unassigned timoreimann May 25, 2021

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 25, 2021

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 25, 2021

srikiz changed the title ~~[Digital Ocean] Code cleanup with no functional modifications~~ [WIP] [Digital Ocean] Code cleanup with no functional modifications May 25, 2021

k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 25, 2021

Move cloud.do from pkg/resources/digitalocean/ckoud.go to upup/pkg/fi…

4cecc64

…/cloudup/do directory

srikiz force-pushed the DO-Use-Interfaces branch from 7ef549b to 4531be5 Compare May 27, 2021 20:53

srikiz changed the title ~~[WIP] [Digital Ocean] Code cleanup with no functional modifications~~ [Digital Ocean] Code cleanup with no functional modifications May 27, 2021

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 27, 2021

johngmyers requested changes May 28, 2021

View reviewed changes

k8s-ci-robot assigned johngmyers May 28, 2021

srikiz force-pushed the DO-Use-Interfaces branch from 14e748a to 737404a Compare May 28, 2021 16:24

srikiz force-pushed the DO-Use-Interfaces branch from 737404a to b12150d Compare May 28, 2021 16:48

Fix upup cloud.go dependencies based on the new interface spec, also …

b5154bb

…update protokube

srikiz force-pushed the DO-Use-Interfaces branch from b12150d to b5154bb Compare May 28, 2021 17:08

johngmyers reviewed May 29, 2021

View reviewed changes

johngmyers approved these changes May 29, 2021

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 29, 2021

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 29, 2021

Incorporate review comments

4f295dc

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 1, 2021

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 1, 2021

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 1, 2021

k8s-ci-robot merged commit 91d8ffe into kubernetes:master Jun 1, 2021

k8s-ci-robot added this to the v1.22 milestone Jun 1, 2021

srikiz deleted the DO-Use-Interfaces branch June 1, 2021 14:19

srikiz mentioned this pull request Aug 18, 2021

[DigitalOcean] Code refactor to use the existing dnsprovider package #12173

Merged

	return nil, errors.New("Not implemented")
	return nil, errors.New("not implemented")

[Digital Ocean] Code cleanup with no functional modifications #11592

[Digital Ocean] Code cleanup with no functional modifications #11592

Conversation

srikiz commented May 24, 2021

johngmyers commented May 24, 2021

timoreimann left a comment

Choose a reason for hiding this comment

timoreimann commented May 25, 2021

johngmyers commented May 25, 2021

srikiz commented May 25, 2021

srikiz commented May 25, 2021

johngmyers commented May 25, 2021

srikiz commented May 26, 2021

johngmyers commented May 26, 2021

srikiz commented May 27, 2021

johngmyers left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

srikiz commented May 28, 2021

johngmyers commented May 28, 2021

srikiz commented May 28, 2021

srikiz commented May 28, 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

johngmyers 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

k8s-ci-robot commented May 29, 2021

johngmyers commented Jun 1, 2021

srikiz commented Jun 1, 2021