TrueCloudLab/frostfs-node
20
1
Fork 27
Code Issues 101 Pull requests 6 Projects Releases 1 Activity Actions

Add staticcheck #203

Merged
fyrchik merged 1 commit from dstepanov-yadro/frostfs-node:refactoring/staticcheck into master 2023-07-26 21:07:57 +00:00
Conversation 7 Commits 1 Files changed 2 +12
dstepanov-yadro commented 2023-04-03 14:06:18 +00:00
Member
Copy link

https://staticcheck.io/ is not the same as golang-cli staticcheck linter.

https://staticcheck.io/ is not the same as golang-cli staticcheck linter.
dstepanov-yadro force-pushed refactoring/staticcheck from 95f67b250f to 7927f48961 2023-04-03 14:08:40 +00:00 Compare
dstepanov-yadro requested review from storage-core-committers 2023-04-03 14:09:09 +00:00
dstepanov-yadro requested review from storage-core-developers 2023-04-03 14:09:09 +00:00
acid-ant commented 2023-04-03 14:34:21 +00:00
Member
Copy link

Typo in commit message "[#203] pilarama: Refactor tests" -> [#203] pilorama...

Typo in commit message "[#203] pilarama: Refactor tests" -> `[#203] pilorama...`
acid-ant approved these changes 2023-04-03 14:41:25 +00:00
carpawell requested review from storage-core-committers 2023-04-03 15:11:39 +00:00
carpawell reviewed 2023-04-03 15:13:27 +00:00
carpawell left a comment
Contributor
Copy link

I do believe it is a good thing but do we have any issue/discussion about that?

Also, we usually end commit messages (body, not header) with a period.

I do believe it is a good thing but do we have any issue/discussion about that? Also, we usually end commit messages (body, not header) with a period.
cmd/frostfs-adm/internal/modules/morph/initialize_nns.go Outdated
@ -286,6 +286,7 @@ func parseNNSResolveResult(res stackitem.Item) (util.Uint160, error) {
func nnsIsAvailable(c Client, nnsHash util.Uint160, name string) (bool, error) {
switch ct := c.(type) {
case *rpcclient.Client:
//lint:ignore SA1019 https://git.frostfs.info/TrueCloudLab/frostfs-node/issues/202
carpawell commented 2023-04-03 15:03:37 +00:00
Contributor
Copy link

can that be fixed in one place like it was already done?

can that be fixed in one place like it was already [done](https://git.frostfs.info/TrueCloudLab/frostfs-node/src/commit/a69c6d1ec9e43f49e6e588d8770ea05ccbcef050/.golangci.yml#L27-L28)?
dstepanov-yadro commented 2023-04-03 16:13:28 +00:00
Author
Member
Copy link

It is possible. But I think that we should not disable this check for the entire project.

It is possible. But I think that we should not disable this check for the entire project.
cmd/frostfs-cli/modules/container/delete.go Outdated
@ -79,3 +79,2 @@
commonCmd.ExitOnErr(cmd, "",
fmt.Errorf("Container wasn't removed because LOCK objects were found.\n"+
"Use --%s flag to remove anyway.", commonflags.ForceFlag))
fmt.Errorf("container wasn't removed because LOCK objects were found.\n"+
carpawell commented 2023-04-03 15:06:09 +00:00
Contributor
Copy link

that is a CLI util output, is that change really necessary?

that is a CLI util output, is that change really necessary?
dstepanov-yadro commented 2023-04-04 06:48:54 +00:00
Author
Member
Copy link

Anyway there is a check for the format of the error text.

Fixed error text format to match other errors.

Anyway there is a check for the format of the error text. Fixed error text format to match other errors.
fyrchik commented 2023-04-04 11:38:54 +00:00
Owner
Copy link

I believe cmd.PrintErrLn called inside ExitOnErr should print Error: prefix.

I believe `cmd.PrintErrLn` called inside `ExitOnErr` should print `Error: ` prefix.
dstepanov-yadro commented 2023-04-04 12:01:40 +00:00
Author
Member
Copy link

There is no prefix

There is no prefix
dstepanov-yadro commented 2023-04-03 16:14:30 +00:00
Author
Member
Copy link

I do believe it is a good thing but do we have any issue/discussion about that?

This is my initiative. We can discuss it here.

> I do believe it is a good thing but do we have any issue/discussion about that? This is my initiative. We can discuss it here.
fyrchik reviewed 2023-04-03 16:22:18 +00:00
Makefile Outdated
@ -134,1 +134,4 @@
# Run staticcheck
staticcheck:
@go install honnef.co/go/tools/cmd/staticcheck@latest
fyrchik commented 2023-04-03 16:22:18 +00:00
Owner
Copy link

Can I still run this target without network connection?

Can I still run this target without network connection?
dstepanov-yadro commented 2023-04-04 07:43:18 +00:00
Author
Member
Copy link

No. Fixed.

No. Fixed.
fyrchik marked this conversation as resolved
realloc commented 2023-04-03 16:39:28 +00:00
Owner
Copy link
  • I'd suggest avoiding local hooks that may be used in other repos in favor of creating our own reusable hook collection in a separate repo
  • Maybe it would be better to transform all lint/check related make targets into pre-commit hooks and make a make target to run pre-commit run -a -hook-stage manual for those who don't like to have pre-commit enabled in local repo clone
- I'd suggest avoiding local hooks that may be used in other repos in favor of creating our own reusable hook collection in a separate repo - Maybe it would be better to transform all lint/check related make targets into pre-commit hooks and make a make target to run `pre-commit run -a -hook-stage manual` for those who don't like to have pre-commit enabled in local repo clone
aarifullin reviewed 2023-04-03 22:17:53 +00:00
pkg/services/control/service_test.go Outdated
@ -86,3 +86,3 @@
info1 := b1.Shards[i].GetBlobstor()
info2 := b2.Shards[i].GetBlobstor()
return compareBlobstorInfo(info1, info2)
if !compareBlobstorInfo(info1, info2) {
aarifullin commented 2023-04-03 22:17:53 +00:00
Member
Copy link

Interesting...

Interesting...
dstepanov-yadro commented 2023-04-04 06:34:00 +00:00
Author
Member
Copy link

False positive?

False positive?
fyrchik commented 2023-04-04 11:35:39 +00:00
Owner
Copy link

I think it is interesting that staticcheck can find this.
The rule is probably "see if we return on every branch in the loop body".

Shouldn't catch this, though:

var x = true
if x {
    return compareBlobstorInfo(info1, info2)
}
I think it is interesting that staticcheck can find this. The rule is probably "see if we return on every branch in the loop body". Shouldn't catch this, though: ``` var x = true if x { return compareBlobstorInfo(info1, info2) } ```
fyrchik marked this conversation as resolved
aarifullin reviewed 2023-04-03 22:22:52 +00:00
pkg/services/object_manager/tombstone/source/source.go Outdated
@ -60,3 +59,3 @@
// Returns nil, nil if the tombstone has been removed
// or marked for removal.
func (s Source) Tombstone(ctx context.Context, a oid.Address, _ uint64) (*object.Object, error) {
func (s Source) Tombstone(ctx context.Context, a oid.Address, _ uint64) (*objectSDK.Object, error) {
aarifullin commented 2023-04-03 22:22:52 +00:00
Member
Copy link

Was that linter suggestion/warning? This PR has truncated *SDK suffix in many places but here it's been added. Maybe we should keep object and just remove objectSDK "git.frostfs.info/TrueCloudLab/frostfs-sdk-go/object"?

Was that linter suggestion/warning? This PR has truncated `*SDK` suffix in many places but here it's been added. Maybe we should keep `object` and just remove `objectSDK "git.frostfs.info/TrueCloudLab/frostfs-sdk-go/object"`?
dstepanov-yadro commented 2023-04-04 06:53:52 +00:00
Author
Member
Copy link

Fixed.

Fixed.
fyrchik commented 2023-04-04 11:36:43 +00:00
Owner
Copy link

We have object package in core and api. To improve readability our unwritten rule is to use objectSDK and objectAPI explicitly.

We have `object` package in core and api. To improve readability our unwritten rule is to use `objectSDK` and `objectAPI` explicitly.
dstepanov-yadro commented 2023-04-04 11:49:35 +00:00
Author
Member
Copy link

Now we know this rule. Fixed to objectSDK.

Now we know this rule. Fixed to objectSDK.
fyrchik commented 2023-04-05 11:12:57 +00:00
Owner
Copy link

We wanted to write a linter for this at one point.

We wanted to write a linter for this at one point.
fyrchik marked this conversation as resolved
dstepanov-yadro force-pushed refactoring/staticcheck from 7927f48961 to 3492eccc2a 2023-04-04 06:37:22 +00:00 Compare
dstepanov-yadro commented 2023-04-04 06:37:30 +00:00
Author
Member
Copy link

Typo in commit message "[#203] pilarama: Refactor tests" -> [#203] pilorama...

fixed

> Typo in commit message "[#203] pilarama: Refactor tests" -> `[#203] pilorama...` fixed
dstepanov-yadro force-pushed refactoring/staticcheck from 3492eccc2a to 54d015c94b 2023-04-04 06:46:18 +00:00 Compare
dstepanov-yadro force-pushed refactoring/staticcheck from 54d015c94b to 7fd95ce34b 2023-04-04 06:52:04 +00:00 Compare
aarifullin approved these changes 2023-04-04 07:04:18 +00:00
aarifullin left a comment
Member
Copy link

LGTM

LGTM
dstepanov-yadro force-pushed refactoring/staticcheck from 7fd95ce34b to 1e7e8d110f 2023-04-04 07:37:38 +00:00 Compare
dstepanov-yadro commented 2023-04-04 07:45:49 +00:00
Author
Member
Copy link
  • I'd suggest avoiding local hooks that may be used in other repos in favor of creating our own reusable hook collection in a separate repo

We will discuss it. Currently replaced local hook with external.

  • Maybe it would be better to transform all lint/check related make targets into pre-commit hooks and make a make target to run pre-commit run -a -hook-stage manual for those who don't like to have pre-commit enabled in local repo clone

Done.

> - I'd suggest avoiding local hooks that may be used in other repos in favor of creating our own reusable hook collection in a separate repo We will discuss it. Currently replaced local hook with external. > - Maybe it would be better to transform all lint/check related make targets into pre-commit hooks and make a make target to run `pre-commit run -a -hook-stage manual` for those who don't like to have pre-commit enabled in local repo clone Done.
dstepanov-yadro commented 2023-04-04 07:47:18 +00:00
Author
Member
Copy link

Typo in commit message "[#203] pilarama: Refactor tests" -> [#203] pilorama...

Fixed

> Typo in commit message "[#203] pilarama: Refactor tests" -> `[#203] pilorama...` Fixed
dstepanov-yadro commented 2023-04-04 07:47:38 +00:00
Author
Member
Copy link

Also, we usually end commit messages (body, not header) with a period.

Fixed.

> Also, we usually end commit messages (body, not header) with a period. Fixed.
dstepanov-yadro force-pushed refactoring/staticcheck from 1e7e8d110f to 13e6c0040a 2023-04-04 11:49:04 +00:00 Compare
dstepanov-yadro force-pushed refactoring/staticcheck from 13e6c0040a to 2f4207b7e8 2023-04-04 13:35:41 +00:00 Compare
aarifullin approved these changes 2023-04-05 13:40:06 +00:00
dstepanov-yadro force-pushed refactoring/staticcheck from 2f4207b7e8 to 9027695371 2023-04-06 13:34:00 +00:00 Compare
fyrchik approved these changes 2023-04-06 14:26:03 +00:00
fyrchik merged commit 9027695371 into master 2023-04-06 14:26:16 +00:00
fyrchik referenced this pull request from a commit 2023-04-06 14:26:17 +00:00
[#203] node: Fix double imports
fyrchik referenced this pull request from a commit 2023-04-06 14:26:17 +00:00
[#203] morph: Mark depracated methods
fyrchik referenced this pull request from a commit 2023-04-06 14:26:17 +00:00
[#203] cli: Fix error message
fyrchik referenced this pull request from a commit 2023-04-06 14:26:17 +00:00
[#203] node: Resolve unused vars
fyrchik referenced this pull request from a commit 2023-04-06 14:26:17 +00:00
[#203] pilorama: Refactor tests
fyrchik referenced this pull request from a commit 2023-04-06 14:26:17 +00:00
[#203] control: Fix test
Sign in to join this conversation.
Reviewers
No reviewers
acid-ant
aarifullin
fyrchik
Labels
Clear labels   
P0

Highest

  
P1

High

  
P2

Medium

  
P3

Low

  
badger

Badger DB related issues

  
frostfs-adm

   
frostfs-cli

   
frostfs-ir

   
frostfs-lens

   
frostfs-node

   
good first issue

Good for newcomers

  
triage
  
Infrastructure

Infrastructure, Automation, CI/CD or DevOps related stuff

  
blocked

The issue or PR is blocked by something

  
bug

Something is not working

  
config

Configuration format update or breaking change

  
discussion

Talking about something in order to reach a decision or to exchange ideas

  
documentation

Documentation related

  
duplicate

This issue or pull request already exists

  
enhancement

New feature

  
go

Language features adoption

  
help wanted

Extra attention is needed

  
internal

   
invalid

Something is wrong

  
kludge

This is a kludge and we know it

  
observability

Related to metrics, tracing and logs

  
perfomance

Perfomance related

  
question

More information is needed

  
refactoring

Refactoring

  
wontfix

This won't be fixed

No labels
P0
P1
P2
P3
badger
frostfs-adm
frostfs-cli
frostfs-ir
frostfs-lens
frostfs-node
good first issue
triage
Infrastructure
blocked
bug
config
discussion
documentation
duplicate
enhancement
go
help wanted
internal
invalid
kludge
observability
perfomance
question
refactoring
wontfix
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
6 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: TrueCloudLab/frostfs-node#203
No description provided.
Delete branch "dstepanov-yadro/frostfs-node:refactoring/staticcheck"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?