-
Notifications
You must be signed in to change notification settings - Fork 152
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
Update kanister build v0.0.20 #1676
Conversation
Thanks for submitting this pull request 🎉. The team will review it soon and get back to you. If you haven't already, please take a moment to review our project contributing guideline and Code of Conduct document. |
pkg/blockstorage/ibm/ibmcloud.go
Outdated
@@ -124,7 +124,7 @@ func (s *ibmCloud) volumeParse(ctx context.Context, vol *ibmprov.Volume) (*block | |||
} | |||
attribs[LunIDAttName] = vol.LunID | |||
|
|||
if len(vol.IscsiTargetIPAddresses) < 0 { | |||
if len(vol.IscsiTargetIPAddresses) == 0 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems like a bug fix that shouldn't be hidden in a tooling bump commit. Can we split this out into a separate PR, please?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pkg/blockstorage/vmware/vmware.go
Outdated
@@ -791,6 +791,7 @@ func (ge govmomiError) Format() string { | |||
return fmt.Sprintf("[%s]", strings.Join(msgs, "; ")) | |||
} | |||
|
|||
// nolint:gocognit |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Directive" type comments shouldn't have a space after the //
.
// nolint:gocognit | |
//nolint:gocognit |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
heh, theres a bunch of places where this needs to be fixed
pkg/envdir/envdir.go
Outdated
fis := make([]fs.FileInfo, 0, len(entries)) | ||
for _, entry := range entries { | ||
fi, err := entry.Info() | ||
if err != nil { | ||
return nil, errors.Wrap(err, "failed to read info from entry:"+entry.Name()) | ||
} | ||
fis = append(fis, fi) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you explain why we're making this change? I don't quite understand.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This deprecation note explains it - https://pkg.go.dev/io/ioutil#ReadDir
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
However I blindly converted. After looking at the code more, I can probably use the response from ReadDir directly.
pkg/kube/pod.go
Outdated
// 1. if PVC is present then check the status of PVC | ||
// 1.1 if PVC is pending then check if the PV status is VolumeFailed return error if so. if not then wait for timeout. | ||
// 2. if PVC not present then wait for timeout |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This reformatting is showing that Go doesn't recognize the nested "1.1" item (Go doesn't support nested lists at all). The way this is formatted, it's actually treating it as a continuation of the 1.
list item — "… the status of PVC 1.1 if PVC is …".
We should probably reformat this to not use a nested list (i.e. move "1.1" to be a top-level item in the list, renumbering the items or switching to an unordered list).
pkg/kube/pod.go
Outdated
// 1. if PVC is present then check the status of PVC | ||
// 1.1 if PVC is pending then check if the PV status is VolumeFailed return error if so. if not then wait for timeout. | ||
// 2. if PVC not present then wait for timeout |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto, as above.
@bathina2 other than Mark's comments, LGTM. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@miquella had additional comments. Please follow up in separate PRs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, just a few tiny Go Doc Comment nits
// - if PVC is present then check the status of PVC | ||
// - if PVC is pending then check if the PV status is VolumeFailed return error if so. if not then wait for timeout. | ||
// - if PVC not present then wait for timeout |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tiny nit: Go Doc Comments says that bullet lists should be indented, so this should be:
// - if PVC is present then check the status of PVC | |
// - if PVC is pending then check if the PV status is VolumeFailed return error if so. if not then wait for timeout. | |
// - if PVC not present then wait for timeout | |
// - if PVC is present then check the status of PVC | |
// - if PVC is pending then check if the PV status is VolumeFailed return error if so. if not then wait for timeout. | |
// - if PVC not present then wait for timeout |
// - if PVC is present then check the status of PVC | ||
// - if PVC is pending then check if the PV status is VolumeFailed return error if so. if not then wait for timeout. | ||
// - if PVC not present then wait for timeout |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto:
// - if PVC is present then check the status of PVC | |
// - if PVC is pending then check if the PV status is VolumeFailed return error if so. if not then wait for timeout. | |
// - if PVC not present then wait for timeout | |
// - if PVC is present then check the status of PVC | |
// - if PVC is pending then check if the PV status is VolumeFailed return error if so. if not then wait for timeout. | |
// - if PVC not present then wait for timeout |
// Verbosef("will delete %d packs and rewrite %d packs, this frees %s\n", | ||
// len(removePacks), len(rewritePacks), formatBytes(uint64(removeBytes))) | ||
// | ||
// len(removePacks), len(rewritePacks), formatBytes(uint64(removeBytes))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tiny nit: this looks like it's supposed to be formatted:
// Verbosef("will delete %d packs and rewrite %d packs, this frees %s\n", | |
// len(removePacks), len(rewritePacks), formatBytes(uint64(removeBytes))) | |
// | |
// len(removePacks), len(rewritePacks), formatBytes(uint64(removeBytes))) | |
// | |
// Verbosef("will delete %d packs and rewrite %d packs, this frees %s\n", | |
// len(removePacks), len(rewritePacks), formatBytes(uint64(removeBytes))) |
Change Overview
update kanister build to
v0.0.19v0.0.20. This updates to go 1.19. It also updates the linter and as leads to several changes across all packages.Pull request type
Please check the type of change your PR introduces:
Issues
Test Plan