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

Fix GetDriverName timeout #239

Merged
merged 1 commit into from
Mar 4, 2019
Merged

Fix GetDriverName timeout #239

merged 1 commit into from
Mar 4, 2019

Conversation

jsafrane
Copy link
Contributor

@jsafrane jsafrane commented Mar 4, 2019

It should be CSI call timeout, not deprecated connection timeout.

cc @pohly

@jsafrane
Fix GetDriverName timeout
ef30b5a 
It should be CSI call timeout, not connection timeout.
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jsafrane

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

@k8s-ci-robot k8s-ci-robot requested review from lpabon and msau42 March 4, 2019 09:25
@k8s-ci-robot k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Mar 4, 2019
pohly
pohly reviewed Mar 4, 2019
View reviewed changes
Copy link
Contributor

@pohly pohly left a comment

Choose a reason for hiding this comment

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

Perhaps change

connectionTimeout = flag.Duration("connection-timeout", 0, "This option is deprecated.")

into

_ = flag.Duration("connection-timeout", 0, "This option is deprecated.")

to prevent accidentally using it?

Just an idea, we can also merge as-is.

@jsafrane
Copy link
Contributor Author

jsafrane commented Mar 4, 2019

There is warning when the option is used and I'd like to keep it there.

external-provisioner/cmd/csi-provisioner/csi-provisioner.go

Lines 80 to 82 in f588078

if *connectionTimeout != 0 {
klog.Warningf("Warning: option -connection-timeout is deprecated and has no effect")
}

pohly
pohly approved these changes Mar 4, 2019
View reviewed changes
Copy link
Contributor

@pohly pohly left a comment

Choose a reason for hiding this comment

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

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 4, 2019
@k8s-ci-robot k8s-ci-robot merged commit 1f293ff into kubernetes-csi:master Mar 4, 2019
pohly added a commit to pohly/external-provisioner that referenced this pull request Mar 18, 2019
@pohly
better deprecated option handling
1ceadfb 
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).
@pohly pohly mentioned this pull request Mar 18, 2019
Merged
pohly added a commit to pohly/external-provisioner that referenced this pull request Mar 18, 2019
@pohly
better deprecated option handling
e03039d 
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).
pohly added a commit to pohly/external-provisioner that referenced this pull request Mar 18, 2019
@pohly
better deprecated flag handling
bfdbff0 
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).
pohly added a commit to pohly/external-provisioner that referenced this pull request Mar 18, 2019
@pohly
better deprecated flag handling
029c447 
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).
pohly added a commit to pohly/csi-lib-utils that referenced this pull request Mar 19, 2019
@pohly
deprecated flag handling
34bfa2b 
Several sidecars (for example, external-provisioner) deprecate command
line flags. When that happens, the flag must still be treated the same
way as before (i.e. optional values for boolean, required values for
non-boolean), but trying to use the flag needs to trigger a
deprecation warning.

An alternative to this code is to let the flag set a variable and then
check the variable. But that only catches cases where the variable is
set to a non-default value and leaves the variable in the code, which
can lead to mistakes (see
kubernetes-csi/external-provisioner#239).
@pohly pohly mentioned this pull request Mar 19, 2019
Merged
pohly added a commit to pohly/external-provisioner that referenced this pull request Mar 29, 2019
@pohly
better deprecated flag handling
bd88f46 
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).
pohly added a commit to pohly/external-provisioner that referenced this pull request Apr 1, 2019
@pohly
better deprecated flag handling + vendor update
d8f2d7e 
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.
kbsonlong pushed a commit to kbsonlong/external-provisioner that referenced this pull request Dec 29, 2023
@k8s-ci-robot
Merge pull request kubernetes-csi#239 from jsafrane/prepare-v3
298cb96 
Prepare v3 release
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pohly pohly pohly approved these changes

@lpabon lpabon Awaiting requested review from lpabon

@msau42 msau42 Awaiting requested review from msau42

Assignees

@pohly pohly

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jsafrane @k8s-ci-robot @pohly