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.

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

rbd: use different variable instead of builtin cap attribute #1613

Merged
merged 2 commits into from Oct 21, 2020

Conversation

humblec
Copy link
Collaborator

@humblec humblec commented Oct 21, 2020

`cap` builtin function returns the capacity of a type. Its not
good practice to use this builtin function for other variable
names, removing it here

Ref# https://golang.org/pkg/builtin/#cap

Signed-off-by: Humble Chirammal hchiramm@redhat.com

@humblec humblec requested a review from nixpanic October 21, 2020 07:01
Copy link
Member

@nixpanic nixpanic left a comment

Choose a reason for hiding this comment

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

Please explain in the commit message why this is needed. Is it because a new Golang version that adds cap as builtin? I do not get any warning/error messages, and CI just works too.

Consider an empty commit message as not acceptable in 99% of all cases.

@humblec
Copy link
Collaborator Author

humblec commented Oct 21, 2020

Please explain in the commit message why this is needed. Is it because a new Golang version that adds cap as builtin? I do not get any warning/error messages, and CI just works too.

Consider an empty commit message as not acceptable in 99% of all cases.

@nixpanic updated the commit message with the details. Please review.

Copy link

@pkalever pkalever left a comment

Choose a reason for hiding this comment

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

@humblec we got few other places

internal/util/validate.go:85:   for _, cap := range req.GetVolumeCapabilities() {
internal/util/validate.go:86:           if m := cap.GetAccessMode().Mode; m == csi.VolumeCapability_AccessMode_MULTI_NODE_READER_ONLY || m == csi.VolumeCapability_AccessMode_SINGLE_NODE_READER_ONLY {

please take care of them too.

@humblec
Copy link
Collaborator Author

humblec commented Oct 21, 2020

@humblec we got few other places

internal/util/validate.go:85:   for _, cap := range req.GetVolumeCapabilities() {
internal/util/validate.go:86:           if m := cap.GetAccessMode().Mode; m == csi.VolumeCapability_AccessMode_MULTI_NODE_READER_ONLY || m == csi.VolumeCapability_AccessMode_SINGLE_NODE_READER_ONLY {

please take care of them too.

Sure... Thanks 👍

@humblec
Copy link
Collaborator Author

humblec commented Oct 21, 2020

@pkalever done.. May be this time the Grep foo worked for me .. I dont see any more such instances, however please confirm from your end too!

@humblec humblec added this to the release-3.2.0 milestone Oct 21, 2020
Copy link
Member

@nixpanic nixpanic left a comment

Choose a reason for hiding this comment

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

There is still a member called cap in internal/csi-common/driver.go for type CSIDriver. This needs changing too.

@humblec
Copy link
Collaborator Author

humblec commented Oct 21, 2020

There is still a member called cap in internal/csi-common/driver.go for type CSIDriver. This needs changing too.

Sure.. Done there too @nixpanic

`cap` builtin function returns the capacity of a type. Its not
good practice to use this builtin function for other variable
names, removing it here

Ref# https://golang.org/pkg/builtin/#cap

Signed-off-by: Humble Chirammal <hchiramm@redhat.com>
Signed-off-by: Humble Chirammal <hchiramm@redhat.com>
@mergify mergify bot merged commit 5ac3f1e into ceph:master Oct 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants