chore: Refactoring utilities to a separate folder to avoid duplication #977

landreasyan · 2021-08-20T00:19:10Z

What type of PR is this?

This PR refactors the utilities functions into a separate directory to facilitate the development of the v2 driver that has dependency on these functions outside of azuredisk package. Refactoring utilities functions into a separate directory avoids duplication of these functions in the v2 driver.

What this PR does / why we need it:

Which issue(s) this PR fixes:

Fixes #

Requirements:

uses conventional commit messages
includes documentation
adds unit tests
tested upgrade from previous version

Special notes for your reviewer:

Release note:

none

k8s-ci-robot · 2021-08-20T00:19:17Z

Hi @landreasyan. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

Adding refactoring changes to v2 files. Refactoring remaining v2 files. Updating formatting.

pkg/azuredisk/azuredisk.go

edreed · 2021-08-23T22:08:15Z

pkg/azureutils/azure_disk_constants.go

+	"regexp"
+)
+
+const (


I wonder if we should move these to their own constants package to consistently access them through that alias rather than both constants and azureutils?

I was also thinking of making constants separate package but left it as this for now. If there are no objections for having them as separate package, I would love to make that change.

I have no objections - I believe it would be a good thing to do. @andyzhangx What do you think?

use consts instead of constants ?

I have refactored constants into a separate pkg.

pkg/azureutils/azure_disk_constants.go

andyzhangx · 2021-08-24T02:18:29Z

/ok-to-test

andyzhangx · 2021-08-24T02:24:31Z

pkg/azureutils/azure_disk_constants.go

+	"regexp"
+)
+
+const (


use consts instead of constants ?

pkg/azuredisk/nodeserver.go

Refactoring constants into a separate package. Refactoring constants into a separate package.

pkg/azuredisk/azuredisk.go

andyzhangx · 2021-08-25T01:17:46Z

/retest

andyzhangx

/lgtm
/approve

andyzhangx · 2021-08-25T14:35:45Z

there is ut failure:

sigs.k8s.io/azuredisk-csi-driver/pkg/azuredisk [sigs.k8s.io/azuredisk-csi-driver/pkg/azuredisk.test]
pkg/azuredisk/controllerserver_test.go:136:8: undefined: logicalSectorSizeField
pkg/azuredisk/controllerserver_test.go:359:8: undefined: skuNameField
pkg/azuredisk/controllerserver_test.go:371:23: undefined: managedDiskPath
pkg/azuredisk/controllerserver_test.go:771:19: undefined: cachingModeField

edreed · 2021-08-25T19:30:41Z

/lgtm
/retest

edreed · 2021-08-25T20:40:40Z

/retest

edreed · 2021-08-25T21:08:19Z

The job pull-azuredisk-csi-driver-verify is failing with the following error:

Verifying gofmt
diff -u ./pkg/azuredisk/fake_azuredisk_v2.go.orig ./pkg/azuredisk/fake_azuredisk_v2.go
--- ./pkg/azuredisk/fake_azuredisk_v2.go.orig	2021-08-25 20:43:08.391083629 +0000
+++ ./pkg/azuredisk/fake_azuredisk_v2.go	2021-08-25 20:43:08.391083629 +0000
@@ -1,3 +1,4 @@
+//go:build azurediskv2
 // +build azurediskv2
...

It looks like Go is changing the format of the build tags from // +build <tag1> <tag2> to //go:build <tag1> || <tag2> enabling the use of Boolean expressions to make it easier to understand when they apply. (See golang/go#41184.) This change is being transitioned in starting with Go 1.17 and it looks like the kubekins-e2e images used to run the Prow jobs have been updated to this version. (kubernetes/test-infra#23354, I think.) Although the Go compiler still accepts 1.16- build tags, gofmt has been updated to flag the usage of the old tags. Thus the reason for the pull-azuredisk-csi-driver-verify failures.

There is a proposed transition here: https://go.googlesource.com/proposal/+/master/design/draft-gobuild.md#transition. Basically, we need to duplicate the old build tags in the new format, e.g.

// +build azurediskv2
//go:build azurediskv2

edreed · 2021-08-25T23:18:06Z

Now verify-boilerplate.sh is failing with the following errors:

Verifying boilerplate
Header in /home/prow/go/src/sigs.k8s.io/azuredisk-csi-driver/hack/../pkg/azuredisk/fake_azuredisk_v2.go does not match reference, diff:

--- reference
+++ /home/prow/go/src/sigs.k8s.io/azuredisk-csi-driver/hack/../pkg/azuredisk/fake_azuredisk_v2.go
@@ -1,3 +1,4 @@
+//go:build azurediskv2
 /*
 Copyright YEAR The Kubernetes Authors.
 
@@ -13,4 +14,3 @@
 See the License for the specific language governing permissions and
 limitations under the License.
 */
-
...

The hack/boilerplate/boilerplate.py script needs to be update to understand the new build tags. A similar change was made in kubernetes/kubernetes#103692 (boilerplate.py).

edreed · 2021-08-26T01:14:23Z

/lgtm

edreed · 2021-08-26T01:14:36Z

/retest

andyzhangx

/approve

k8s-ci-robot · 2021-08-26T01:39:29Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andyzhangx, landreasyan

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

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

landreasyan added 2 commits August 19, 2021 17:07

Refactoring utilities functions to a separate folder.

abd1f0d

Merge branch 'master' into azuredisk-utilities-refactoring

ece25af

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Aug 20, 2021

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Aug 20, 2021

k8s-ci-robot requested review from andyzhangx and ZeroMagic August 20, 2021 00:19

k8s-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Aug 20, 2021

landreasyan changed the title ~~chor: Refactoring utilities to a separate folder to avoid duplication~~ chore: Refactoring utilities to a separate folder to avoid duplication Aug 20, 2021

landreasyan marked this pull request as draft August 20, 2021 00:47

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 Aug 20, 2021

Refactoring v2 files.

76da6fe

Adding refactoring changes to v2 files. Refactoring remaining v2 files. Updating formatting.

landreasyan force-pushed the azuredisk-utilities-refactoring branch from 2a12f3b to 76da6fe Compare August 20, 2021 02:07

landreasyan marked this pull request as ready for review August 20, 2021 02:23

k8s-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels Aug 20, 2021

Merge branch 'master' into azuredisk-utilities-refactoring

704d7a8

k8s-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Aug 20, 2021

Merge branch 'master' into azuredisk-utilities-refactoring

b384410

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 23, 2021

edreed reviewed Aug 23, 2021

View reviewed changes

pkg/azuredisk/azuredisk.go Show resolved Hide resolved

edreed reviewed Aug 23, 2021

View reviewed changes

pkg/azureutils/azure_disk_constants.go Outdated Show resolved Hide resolved

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Aug 24, 2021

andyzhangx reviewed Aug 24, 2021

View reviewed changes

edreed mentioned this pull request Aug 24, 2021

Support UserAgent Attribution #962

Closed

4 tasks

landreasyan force-pushed the azuredisk-utilities-refactoring branch from cc38df9 to 961207e Compare August 24, 2021 23:43

Refactoring constants into a separate package

3efbcba

Refactoring constants into a separate package. Refactoring constants into a separate package.

landreasyan force-pushed the azuredisk-utilities-refactoring branch from 961207e to 3efbcba Compare August 24, 2021 23:45

edreed reviewed Aug 25, 2021

View reviewed changes

pkg/azuredisk/azuredisk.go Outdated Show resolved Hide resolved

andyzhangx approved these changes Aug 25, 2021

View reviewed changes

k8s-ci-robot assigned andyzhangx Aug 25, 2021

k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Aug 25, 2021

landreasyan added 2 commits August 25, 2021 10:32

Merge branch 'master' into azuredisk-utilities-refactoring

2130dee

Addressing failures caused by automatic merge.

90ca7a7

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

k8s-ci-robot assigned edreed Aug 25, 2021

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

Duplicating go build tags in new format.

6046991

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

Updating bolilerplate.py to recognize new version of go build tags.

eeaae31

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

andyzhangx approved these changes Aug 26, 2021

View reviewed changes

k8s-ci-robot merged commit 6663aee into kubernetes-sigs:master Aug 26, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: Refactoring utilities to a separate folder to avoid duplication #977

chore: Refactoring utilities to a separate folder to avoid duplication #977

landreasyan commented Aug 20, 2021 •

edited

Loading

k8s-ci-robot commented Aug 20, 2021

edreed Aug 23, 2021

landreasyan Aug 23, 2021

edreed Aug 23, 2021 •

edited

Loading

andyzhangx Aug 24, 2021

landreasyan Aug 24, 2021

andyzhangx commented Aug 24, 2021

andyzhangx Aug 24, 2021

andyzhangx commented Aug 25, 2021

andyzhangx left a comment

andyzhangx commented Aug 25, 2021

edreed commented Aug 25, 2021

edreed commented Aug 25, 2021

edreed commented Aug 25, 2021 •

edited

Loading

edreed commented Aug 25, 2021

edreed commented Aug 26, 2021

edreed commented Aug 26, 2021

andyzhangx left a comment

k8s-ci-robot commented Aug 26, 2021

chore: Refactoring utilities to a separate folder to avoid duplication #977

chore: Refactoring utilities to a separate folder to avoid duplication #977

Conversation

landreasyan commented Aug 20, 2021 • edited Loading

k8s-ci-robot commented Aug 20, 2021

edreed Aug 23, 2021

Choose a reason for hiding this comment

landreasyan Aug 23, 2021

Choose a reason for hiding this comment

edreed Aug 23, 2021 • edited Loading

Choose a reason for hiding this comment

andyzhangx Aug 24, 2021

Choose a reason for hiding this comment

landreasyan Aug 24, 2021

Choose a reason for hiding this comment

andyzhangx commented Aug 24, 2021

andyzhangx Aug 24, 2021

Choose a reason for hiding this comment

andyzhangx commented Aug 25, 2021

andyzhangx left a comment

Choose a reason for hiding this comment

andyzhangx commented Aug 25, 2021

edreed commented Aug 25, 2021

edreed commented Aug 25, 2021

edreed commented Aug 25, 2021 • edited Loading

edreed commented Aug 25, 2021

edreed commented Aug 26, 2021

edreed commented Aug 26, 2021

andyzhangx left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Aug 26, 2021

landreasyan commented Aug 20, 2021 •

edited

Loading

edreed Aug 23, 2021 •

edited

Loading

edreed commented Aug 25, 2021 •

edited

Loading