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 more lint errors from goreportcard #34

Closed
wants to merge 6 commits into from
Closed

Fix more lint errors from goreportcard #34

wants to merge 6 commits into from

Conversation

wongni
Copy link
Contributor

@wongni wongni commented Feb 21, 2022 •
edited
Loading

Issue #, if available:

Description of changes:
Fix misspellings
Fix a few comments to satisfy golint checker
Pull getAffinityGroupIDs function to reduce cyclomatic complexity of GetOrCreateVMInstance

Somehow I was not able to buile goreportcard-cli so I used the following method to run the checks locally. (gojp/goreportcard#379)

❯ docker run -it --rm -v ~/dev/pike/cluster-api-provider-cloudstack:/cluster-api-provider-cloudstack  docker.io/jenner/goreportcard-cli:latest /bin/sh -c "cd /cluster-api-provider-cloudstack && goreportcard-cli -v"

Current report from this PR

Grade: A+ (99.2%)
Files: 35
Issues: 1
gofmt: 100%
go_vet: 97%
        ../go/pkg/mod/k8s.io/client-go@v0.22.2/plugin/pkg/client/auth/exec/metrics.go
                Line 21: error: package io/fs is not in GOROOT (/usr/local/go/src/io/fs) (vet)
gocyclo: 100%
golint: 100%
ineffassign: 100%
license: 100%
misspell: 100%

Testing performed:
Full E2E test results
Ran 13 of 14 Specs in 5094.519 seconds
SUCCESS! -- 13 Passed | 0 Failed | 0 Pending | 1 Skipped

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@wongni
Fix misspellings
0f34ecf 
Fix a few comments to satisfy golint checker
Pull getAffinityGroupIDs function to reduce cyclomatic complexity of GetOrCreateVMInstance
@aws-paris-bot aws-paris-bot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Feb 21, 2022
@aws-paris-bot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rejoshed, wongni

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

@aws-paris-bot aws-paris-bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Feb 24, 2022
rejoshed
rejoshed reviewed Feb 24, 2022
View reviewed changes
api/v1beta1/cloudstackmachine_webhook.go Outdated
@@ -32,6 +32,7 @@ import (
// log is for logging in this package.
var cloudstackmachinelog = logf.Log.WithName("cloudstackmachine-resource")

// SetupWebhookWithManager creates a new webhook managed by passed manager
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you please add periods to the end of these comments? I'd like us to stick to the prevailing conventions.

rejoshed
rejoshed reviewed Feb 24, 2022
View reviewed changes
pkg/cloud/affinity_groups.go Outdated
type AffinityGroup struct {
Type string
Name string
ID string
}

// AffinityGroupIface contains the collection of functions for AffinityGroup
Copy link
Contributor

Choose a reason for hiding this comment

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

Here too, and probably elsewhere.

rejoshed
rejoshed reviewed Feb 24, 2022
View reviewed changes
pkg/cloud/affinity_groups_test.go Outdated
@@ -14,13 +14,13 @@ See the License for the specific language governing permissions and
limitations under the License.
*/

package cloud_test
package cloud
Copy link
Contributor

Choose a reason for hiding this comment

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

No! We separate this on purpose.

cloud_test being a separate module helps us to detect code smell in un-testable regions.

It also helps us to pair down what libs actually make it into our final module.

Why was this change made?

rejoshed
rejoshed reviewed Feb 24, 2022
View reviewed changes
pkg/cloud/affinity_groups_test.go Outdated
@@ -32,22 +32,22 @@ var _ = Describe("AffinityGroup Unit Tests", func() {
mockCtrl *gomock.Controller
mockClient *cloudstack.CloudStackClient
ags *cloudstack.MockAffinityGroupServiceIface
fakeAG *cloud.AffinityGroup
fakeAG *AffinityGroup
Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, not great.

@wongni wongni requested a review from rejoshed February 24, 2022 20:03
rejoshed
rejoshed reviewed Feb 24, 2022
View reviewed changes
pkg/cloud/affinity_groups_test.go
@@ -47,7 +48,7 @@ var _ = Describe("AffinityGroup Unit Tests", func() {
client = cloud.NewClientFromCSAPIClient(mockClient)
fakeAG = &cloud.AffinityGroup{
Name: "FakeAffinityGroup",
Type: cloud.AffinityGroupType}
Type: "host affinity"}
Copy link
Contributor

Choose a reason for hiding this comment

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

Please revert.

rejoshed
rejoshed reviewed Feb 24, 2022
View reviewed changes
pkg/cloud/affinity_groups.go
@@ -21,17 +21,14 @@ import (
"github.com/pkg/errors"
)

const (
Copy link
Contributor

Choose a reason for hiding this comment

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

Please revert this too. Mark intentionally made these consts.

I don't particularly care whether there are string literals in the tests or not, but these changes will only aggravate the rebase I will have to do to brink in my dummies module.

rejoshed
rejoshed reviewed Feb 24, 2022
View reviewed changes
pkg/cloud/affinity_groups_test.go
@@ -89,7 +90,7 @@ var _ = Describe("AffinityGroup Unit Tests", func() {
if connectionErr != nil { // Only do these tests if an actual ACS instance is available via cloud-config.
Skip("Could not connect to ACS instance.")
}
arbitraryAG = &cloud.AffinityGroup{Name: "ArbitraryAffinityGroup", Type: cloud.AffinityGroupType}
arbitraryAG = &cloud.AffinityGroup{Name: "ArbitraryAffinityGroup", Type: "host affinity"}
Copy link
Contributor

Choose a reason for hiding this comment

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

Please revert.

@aws-paris-bot
Copy link

@wongni: The following tests failed, say /retest to rerun all failed tests:

Test name Commit Details Rerun command
e2e 0f34ecf link /test e2e
capi-provider-cloudstack-presubmit-test 0f34ecf link /test capi-provider-cloudstack-presubmit-test
e2e-pr-blocking ae0ad01 link /test e2e-pr-blocking

Full PR test history. Your PR dashboard.

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/test-infra repository. I understand the commands that are listed here.

@jweite-amazon
Copy link
Contributor

jweite-amazon commented Feb 25, 2022 •
edited
Loading

I'm good with this all at my level, pending resolution of Josh's more detailed points, passing of tests and a mergability.

@wongni
Copy link
Contributor Author

wongni commented Mar 16, 2022

Created #52 from the latest main branch.

@wongni wongni closed this Mar 16, 2022
@wongni wongni deleted the fix/lint_errors branch March 16, 2022 18:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rejoshed rejoshed rejoshed approved these changes

@jweite-amazon jweite-amazon Awaiting requested review from jweite-amazon

@maxdrib maxdrib Awaiting requested review from maxdrib

Assignees
No one assigned
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@wongni @aws-paris-bot @jweite-amazon @rejoshed