better deprecated option handling #255

pohly · 2019-03-18T13:14:50Z

The warning for the deprecated options was only printed
when it was set to a non-default value. When set explicitly to
zero or empty (admittedly unlikely), no warning was printed.

This approach also gets rid of the actual variables and thus catches
code which accidentally still uses it (see
#239).

pohly · 2019-03-18T13:16:29Z

@jsafrane this is an idea that I just had. Not sure whether it's worthwhile.

davidz627

I agree with your premise that we could do a better job handling the deprecated options. However, I think maybe we could simplify the solution. Please see the comment which I tagged "This comment supersedes all my other comments."

davidz627 · 2019-03-18T18:58:29Z

cmd/csi-provisioner/csi-provisioner.go

@@ -68,6 +81,8 @@ func init() {
 	var config *rest.Config
 	var err error

+	deprecatedOption{option: "connection-timeout"}.Define()


these should be defined at the same place as the flags to keep all flag's consolidated

davidz627 · 2019-03-18T19:00:55Z

cmd/csi-provisioner/csi-provisioner.go

+// deprecatedOption can be used to define a deprecated command line
+// flag. Whenever it is set, regardless to which value, a warning is
+// triggered.
+type deprecatedOption struct {


I think these could exist in a different util type package. Maybe a specific file for it

davidz627 · 2019-03-18T19:02:04Z

cmd/csi-provisioner/csi-provisioner.go

+}
+
+func (d deprecatedOption) String() string { return "" }
+func (d deprecatedOption) Set(value string) error {


could you be explicit in what interface this is implementing for strict interface checking (mostly for code maintainability and my understanding)
e.g:
var _ InterfaceName := &deprecatedOption{}

davidz627 · 2019-03-18T19:03:24Z

cmd/csi-provisioner/csi-provisioner.go

+	return nil
+}
+func (d deprecatedOption) Type() string { return "" }
+func (d deprecatedOption) Define()      { flag.Var(d, d.option, "This option is deprecated.") }


could the implementation of Define just live inside Set instead?

davidz627 · 2019-03-18T19:06:01Z

cmd/csi-provisioner/csi-provisioner.go

+	klog.Warningf("Warning: option %s=%q is deprecated and has no effect", d.option, value)
+	return nil
+}
+func (d deprecatedOption) Type() string { return "" }


This comment supersedes all my other comments.

in general I'm not sure why we're using a struct here when maybe just a function will do. I think I would prefer that the definition of these deprecated flags more closely mirrored that of a normal flag. Maybe something as simple that would be invoked like this:
connectionTimeout = deprecatedflag.Set("connection-timeout")

Yes, the API can be made nicer. Let me try something along those lines.

@davidz627 please have another look. I moved the code into an utility package with a single function as interface. Actually, it's two now, because I also added support for boolean flags.

davidz627

mostly lgtm just some final comments/questions

pkg/deprecatedflags/deprecatedflags.go

cmd/csi-provisioner/csi-provisioner.go

pkg/deprecatedflags/deprecatedflags.go

davidz627 · 2019-03-18T21:07:01Z

pkg/deprecatedflags/deprecatedflags.go

+	return nil
+}
+func (d deprecated) Type() string     { return "" }
+func (d deprecated) IsBoolFlag() bool { return d.isBool }


same question, why do we need this?

IsBoolFlag is how the flag package determines whether a value needs the special bool handling.

pkg/deprecatedflags/deprecatedflags.go

pohly

Force-pushed.

cmd/csi-provisioner/csi-provisioner.go

pohly · 2019-03-19T08:32:58Z

pkg/deprecatedflags/deprecatedflags.go

+	return nil
+}
+func (d deprecated) Type() string     { return "" }
+func (d deprecated) IsBoolFlag() bool { return d.isBool }


IsBoolFlag is how the flag package determines whether a value needs the special bool handling.

pkg/deprecatedflags/deprecatedflags.go

jsafrane · 2019-03-19T09:34:47Z

I'd appreciate this in csi-lib-utils, because more sidecars have deprecated --connection-timeout.

pohly · 2019-03-19T10:01:43Z

I'll move the utility package to csi-lib-utils.

This switches to the latest external-snapshotter (picked from the master branch for now). This is necessary because the latest official release is no longer compatible with other dependencies (different client-go versions). Key changes: (1/9) Wrote k8s.io/utils@master: new project (2/9) Wrote k8s.io/[email protected]: version changed (was kubernetes-1.13.0-beta.1) (3/9) Wrote k8s.io/[email protected]: version changed (was kubernetes-1.13.0-beta.1) (4/9) Wrote github.com/kubernetes-csi/external-snapshotter@master: version changed (was v1.0.1) (5/9) Wrote k8s.io/[email protected]: version changed (was kubernetes-1.13.0-beta.1)

xing-yang · 2019-03-29T14:24:41Z

Gopkg.toml

+# TODO: remove when released
+[[constraint]]
+  name = "github.com/kubernetes-csi/external-snapshotter"
+  branch = "master"


Setting it to master is not safe though. Maybe we should cut a release of external-snapshotter first.

I think it is fine. The lock file ensures that we keep on using the correct commit. And updates are controlled

I agree. Whoever does the next dep ensure -update will have to check the TODOs, and thus will ensure that we do not update beyond the next external-snapshotter release.

The warning for the deprecated connection-timeout was only printed when it was set to a non-default value. When set explicitly to zero (admittedly unlikely), no warning was printed. This approach also gets rid of the actual variable and thus catches code which accidentally still uses it (see kubernetes-csi#239). This depends on a more recent csi-lib-utils. Because the previous vendoring was done with an older dep version, "dep ensure -update" cannot update just that project, so everything gets updated instead.

pohly · 2019-04-01T19:15:19Z

@msau42 force-pushed a version which passes "make test" and "make build" locally.

msau42 · 2019-04-01T20:30:27Z

/lgtm
/approve

k8s-ci-robot · 2019-04-01T20:30:40Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: msau42, pohly

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

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

379a1bb9b Merge pull request kubernetes-csi#255 from humblec/sidecar-md a5667bbbb fix typo in sidecar release process 49676850e Merge pull request kubernetes-csi#254 from bells17/add-github-actions d9bd160c2 Update skip list in codespell GitHub Action f5aebfc9f Add GitHub Actions workflows git-subtree-dir: release-tools git-subtree-split: 379a1bb9b001c0d62a091a21f1a4efaf42987248

f40f0ccd4 Merge pull request kubernetes-csi#256 from solumath/master cfa92106c Instruction update 379a1bb9b Merge pull request kubernetes-csi#255 from humblec/sidecar-md a5667bbbb fix typo in sidecar release process 49676850e Merge pull request kubernetes-csi#254 from bells17/add-github-actions d9bd160c2 Update skip list in codespell GitHub Action adb3af9df Merge pull request kubernetes-csi#252 from bells17/update-go-version f5aebfc9f Add GitHub Actions workflows b82ee3888 Merge pull request kubernetes-csi#253 from bells17/fix-typo c31745621 Fix typo 0a7850561 Bump to Go 1.22.3 edd89ad58 Merge pull request kubernetes-csi#251 from jsafrane/add-logcheck 043fd0991 Add test-logcheck target d7535ae0c Merge pull request kubernetes-csi#250 from jsafrane/go-1.22 b52e7ad35 Update go to 1.22.2 14fdb6f66 Merge pull request kubernetes-csi#247 from msau42/prow 9b4352e9f Update release playbook c7bb972cc Fix release notes script to use fixed tags 463a0e9f5 Add script to update specific go modules git-subtree-dir: release-tools git-subtree-split: f40f0ccd458f2d4555e3ca98d69b5a984bae0f14

988496a1f Merge pull request kubernetes-csi#257 from jakobmoellerdev/csi-prow-sidecar-e2e-path 028f8c698 chore: bump to Go 1.22.5 69bd71e8a chore: add CSI_PROW_SIDECAR_E2E_PATH f40f0ccd4 Merge pull request kubernetes-csi#256 from solumath/master cfa92106c Instruction update 379a1bb9b Merge pull request kubernetes-csi#255 from humblec/sidecar-md a5667bbbb fix typo in sidecar release process 49676850e Merge pull request kubernetes-csi#254 from bells17/add-github-actions d9bd160c2 Update skip list in codespell GitHub Action f5aebfc9f Add GitHub Actions workflows git-subtree-dir: release-tools git-subtree-split: 988496a1fc3849ed793e03012fdd56813d13d46c

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Mar 18, 2019

k8s-ci-robot requested review from jsafrane and msau42 March 18, 2019 13:14

k8s-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Mar 18, 2019

pohly force-pushed the deprecated-option branch from 1ceadfb to e03039d Compare March 18, 2019 13:15

davidz627 reviewed Mar 18, 2019

View reviewed changes

pohly force-pushed the deprecated-option branch from e03039d to bfdbff0 Compare March 18, 2019 20:25

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Mar 18, 2019

pohly force-pushed the deprecated-option branch from bfdbff0 to 029c447 Compare March 18, 2019 20:27

davidz627 reviewed Mar 18, 2019

View reviewed changes

pohly commented Mar 19, 2019

View reviewed changes

pohly mentioned this pull request Mar 19, 2019

deprecated flag handling kubernetes-csi/csi-lib-utils#22

Merged

pohly force-pushed the deprecated-option branch from 029c447 to bd88f46 Compare March 29, 2019 09:10

k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Mar 29, 2019

xing-yang reviewed Mar 29, 2019

View reviewed changes

pohly force-pushed the deprecated-option branch from bd88f46 to d8f2d7e Compare April 1, 2019 19:14

k8s-ci-robot assigned msau42 Apr 1, 2019

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

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 1, 2019

k8s-ci-robot merged commit 918496d into kubernetes-csi:master Apr 1, 2019

ialidzhikov mentioned this pull request Mar 31, 2020

Specify --provisioner flag only for csi-provisioner < v1.1.0 gardener/gardener-extension-provider-alicloud#60

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

better deprecated option handling #255

better deprecated option handling #255

pohly commented Mar 18, 2019

pohly commented Mar 18, 2019

davidz627 left a comment

davidz627 Mar 18, 2019

davidz627 Mar 18, 2019

davidz627 Mar 18, 2019

davidz627 Mar 18, 2019

davidz627 Mar 18, 2019

pohly Mar 18, 2019

pohly Mar 18, 2019

davidz627 left a comment

davidz627 Mar 18, 2019

pohly Mar 19, 2019

pohly left a comment

pohly Mar 19, 2019

jsafrane commented Mar 19, 2019

pohly commented Mar 19, 2019

xing-yang Mar 29, 2019

msau42 Mar 29, 2019

pohly Mar 29, 2019

pohly commented Apr 1, 2019

msau42 commented Apr 1, 2019

k8s-ci-robot commented Apr 1, 2019

better deprecated option handling #255

better deprecated option handling #255

Conversation

pohly commented Mar 18, 2019

pohly commented Mar 18, 2019

davidz627 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

davidz627 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pohly left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsafrane commented Mar 19, 2019

pohly commented Mar 19, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pohly commented Apr 1, 2019

msau42 commented Apr 1, 2019

k8s-ci-robot commented Apr 1, 2019