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

fix: Fix code errors reported by golangci-lint #57

Merged
merged 2 commits into from
Nov 10, 2021

Conversation

pugangxa
Copy link
Contributor

Motivation

In #53 reported some issues, checked and fixed the ones that are code errors

Modifications

Fix bug

Result

The code error is fixed, but

  • The field alignment is not fixed, not sure whether it's needed and it may have potential impact, maybe just change the golangci-lint rule to ignore it?
  • Does not upgrade golangci-lint to 1.42.1 and golang to 1.7 since as mentioned it will be looked separately.

@pugangxa
Copy link
Contributor Author

pugangxa commented Oct 19, 2021

Let me know if we want to address the field alignment issue or golang upgrade issue.

Copy link
Member

@pvaneck pvaneck left a comment

Choose a reason for hiding this comment

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

Thanks! Seems like the unusedwrite lint errors are pretty notable and should be caught by the linter. I'm thinking it's worthwhile to update the golangci-lint version to the latest.

As for fieldalignment errors, I'm not sure if the gain from reordering struct fields is worth it here. I know that some of our structs have fields organized in a specific matter based on topic/function. Checking the existing .golangci.yaml, we have the maligned check disabled which is what fieldalignment replaced in the newer golangci-lint versions. So maybe we also disable govet fieldalignment check?

@njhill WDYT?

@pugangxa
Copy link
Contributor Author

Upgraded golangci and disabled fieldalignment.

@pvaneck
Copy link
Member

pvaneck commented Oct 27, 2021

Thanks!
/lgtm

Copy link
Member

@njhill njhill left a comment

Choose a reason for hiding this comment

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

Thanks @pugangxa, and sorry I missed this earlier. These are legit bugs.

@pvaneck I agree with disabling the fieldalignment checks.

Comment on lines 42 to 46
for i, volumeMount := range container.VolumeMounts {
if volumeMount.Name == etcdVolume {
volumeMountExists = true
volumeMount.ReadOnly = true
volumeMount.MountPath = etcdMountPath
container.VolumeMounts[i].ReadOnly = true
container.VolumeMounts[i].MountPath = etcdMountPath
Copy link
Member

Choose a reason for hiding this comment

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

May as well avoid the struct copy altogether and also change to:

			for i, _ := range container.VolumeMounts {
				if container.VolumeMounts[i].Name == etcdVolume {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, addressed.

Copy link
Member

@njhill njhill left a comment

Choose a reason for hiding this comment

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

Thanks @pugangxa!

For future reference, it's best not to modify existing commits that have already been reviewed (apart from rebasing them), just push new commits.

@kserve-oss-bot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: njhill, pugangxa

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

@njhill njhill changed the title fix code error reported by golangci-lint fix: Fix code errors reported by golangci-lint Nov 10, 2021
@njhill
Copy link
Member

njhill commented Nov 10, 2021

/lgtm

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants