-
Notifications
You must be signed in to change notification settings - Fork 91
Fix revive package name conventions #2464
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
Conversation
✅ Deploy Preview for kubernetes-sigs-cluster-api-ibmcloud ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Welcome @mayuka-c! |
Hi @mayuka-c. 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 Once the patch is verified, the new status will be reflected by the 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-sigs/prow repository. |
/ok-to-test |
419b5d8
to
7a47110
Compare
cloud/scope/powervs_cluster.go
Outdated
@@ -49,16 +49,16 @@ import ( | |||
v1beta1patch "sigs.k8s.io/cluster-api/util/deprecated/v1beta1/patch" //nolint:staticcheck | |||
|
|||
infrav1 "sigs.k8s.io/cluster-api-provider-ibmcloud/api/v1beta2" | |||
genUtil "sigs.k8s.io/cluster-api-provider-ibmcloud/genutil" |
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.
why genUtil
and not use genutil
as it is without alias?
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.
Done, I have removed the named import. Thanks!
@@ -35,7 +35,7 @@ import ( | |||
"sigs.k8s.io/controller-runtime/pkg/webhook/admission" | |||
|
|||
infrav1 "sigs.k8s.io/cluster-api-provider-ibmcloud/api/v1beta2" | |||
genUtil "sigs.k8s.io/cluster-api-provider-ibmcloud/util" | |||
genUtil "sigs.k8s.io/cluster-api-provider-ibmcloud/genutil" |
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.
why genUtil
and not use genutil
as it is without alias?
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.
Done
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.
missing supporting doc.go
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.
Since there was only single file, I had added the package comment in the same file. Now I have created doc.go and moved the package comment there
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.
Most of the code under pkg/cloud/services/
are related to the one talking to the cloud, can we move this to somewhere else like pkg/utils
and rename fix the naming aspect.
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.
I have moved the pagining utils under pkg/ dir. Thanks!
cmd/capibmadm/pointer/pointer.go
Outdated
@@ -0,0 +1,54 @@ | |||
/* | |||
Copyright 2023 The Kubernetes Authors. |
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.
Copyright 2023 The Kubernetes Authors. | |
Copyright 2025 The Kubernetes Authors. |
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.
Done thanks
7a47110
to
4da6308
Compare
genutil/doc.go
Outdated
// Package utils implements utils code. | ||
package utils | ||
// Package genutil implements general utility code. | ||
package genutil |
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.
let us avoid having anything in the root folder, see if we can move this under pkg to internal
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.
Have moved it under internal
72f3153
to
8f5bee1
Compare
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.
overall LGTM with a minor comment
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.
Most of the code under pkg/cloud/services/
are related to the one talking to the cloud, can we move this to somewhere else like pkg/utils
and rename fix the naming aspect.
8f5bee1
to
1aae795
Compare
Dockerfile Address comments Comment Dockerfile Comment
1aae795
to
e2aeb43
Compare
I have moved the pagining utils under pkg/ dir. Thanks! |
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
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mayuka-c, mkumatag 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 |
What this PR does / why we need it:
This PR addresses the revive package naming convention issue
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)
format, will close the issue(s) when PR gets merged):Fixes #2458
Special notes for your reviewer:
/kind cleanup
/area provider/ibmcloud
This PR is for fixing the linting issue.
Release note:
Validation
After running
make lint
INFO [runner] linters took 5.793821833s with stages: goanalysis_metalinter: 5.771707958s 0 issues. INFO File cache stats: 3 entries of total size 17.5KiB INFO Memory: 72 samples, avg is 1216.4MB, max is 2175.7MB INFO Execution took 7.086381416s