[Tooling] Add protocheck status-errors subcmd to check for non-gRPC status error returns
#929
Conversation
bryanchriswhite
added
on-chain
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| "fmt" | ||
| "go/ast" | ||
| "go/types" | ||
| "log" |
There was a problem hiding this comment.
[linter-name (fail-on-found)] reported by reviewdog 🐶
"log"
| rootCmd.AddCommand(statusErrorsCheckCmd) | ||
| } | ||
|
|
||
| // TODO_IN_THIS_COMMIT: pre-run: drop patch version in go.mod; post-run: restore. |
There was a problem hiding this comment.
[linter-name (fail-on-found)] reported by reviewdog 🐶
// TODO_IN_THIS_COMMIT: pre-run: drop patch version in go.mod; post-run: restore.
There was a problem hiding this comment.
UPDATE: No need to do this. The reason I had to do this was because I had an old version (1.20.x) of go in my path which was being used by golang.org/x/tools/packages. Pre 1.21.x, go.mod version did not have a patch version.
| func runStatusErrorsCheck(cmd *cobra.Command, _ []string) error { | ||
| ctx := cmd.Context() | ||
|
|
||
| // TODO_IN_THIS_COMMIT: add hack/work-around to temporarily strip patch version from go.mod. |
There was a problem hiding this comment.
[linter-name (fail-on-found)] reported by reviewdog 🐶
// TODO_IN_THIS_COMMIT: add hack/work-around to temporarily strip patch version from go.mod.
There was a problem hiding this comment.
| // TODO_IN_THIS_COMMIT: add hack/work-around to temporarily strip patch version from go.mod. | ||
| // TODO_IN_THIS_COMMIT: add hack/work-around to temporarily strip patch version from go.mod. | ||
|
|
||
| // TODO_IN_THIS_COMMIT: to support this, need to load all modules but only inspect target module. |
There was a problem hiding this comment.
[linter-name (fail-on-found)] reported by reviewdog 🐶
// TODO_IN_THIS_COMMIT: to support this, need to load all modules but only inspect target module.
| } | ||
| // --- END | ||
|
|
||
| // TODO_IN_THIS_COMMIT: extract --- BEGIN |
There was a problem hiding this comment.
[linter-name (fail-on-found)] reported by reviewdog 🐶
// TODO_IN_THIS_COMMIT: extract --- BEGIN
| // --- END | ||
|
|
||
| // TODO_IN_THIS_COMMIT: extract --- BEGIN | ||
| // TODO_IN_THIS_COMMIT: figure out why there are duplicate offending lines. |
There was a problem hiding this comment.
[linter-name (fail-on-found)] reported by reviewdog 🐶
// TODO_IN_THIS_COMMIT: figure out why there are duplicate offending lines.
| // TODO_IN_THIS_COMMIT: extract --- BEGIN | ||
| // TODO_IN_THIS_COMMIT: figure out why there are duplicate offending lines. | ||
| // Print offending lines in package | ||
| // TODO_IN_THIS_COMMIT: refactor to const. |
There was a problem hiding this comment.
[linter-name (fail-on-found)] reported by reviewdog 🐶
// TODO_IN_THIS_COMMIT: refactor to const.
| return | ||
| } | ||
|
|
||
| // TODO_IN_THIS_COMMIT: Implement & log when this happens. |
There was a problem hiding this comment.
[linter-name (fail-on-found)] reported by reviewdog 🐶
// TODO_IN_THIS_COMMIT: Implement & log when this happens.
| return strings.Repeat(" ", indent) | ||
| } | ||
|
|
||
| // TODO_IN_THIS_COMMIT: remove or move to a testutil package. |
There was a problem hiding this comment.
[linter-name (fail-on-found)] reported by reviewdog 🐶
// TODO_IN_THIS_COMMIT: remove or move to a testutil package.
bryanchriswhite
changed the title
protocheck status-errors subcommand to check for non-gRPC status error returns
bryanchriswhite
changed the title
protocheck status-errors subcommand to check for non-gRPC status error returnsprotocheck status-errors subcmd to check for non-gRPC status error returns
go.mod
Outdated
| go 1.23.0 | ||
| go 1.23 |
| } | ||
| //logger.Debug().Msgf("tracing expression args: %+v", expr) | ||
| //for _, arg := range expr.Args { | ||
| // // TODO_IN_THIS_COMMIT: return traceExpressionStack... ? |
There was a problem hiding this comment.
[linter-name (fail-on-found)] reported by reviewdog 🐶
// // TODO_IN_THIS_COMMIT: return traceExpressionStack... ?
| return true | ||
| case *ast.BinaryExpr: | ||
| logger.Debug().Msg("tracing binary expression") | ||
| // TODO_IN_THIS_COMMIT: return traceExpressionStack... ? |
There was a problem hiding this comment.
[linter-name (fail-on-found)] reported by reviewdog 🐶
// TODO_IN_THIS_COMMIT: return traceExpressionStack... ?
| var ( | ||
| flagModule = "module" | ||
| flagModuleShorthand = "m" | ||
| // TODO_IN_THIS_COMMIT: support this flag. |
There was a problem hiding this comment.
[linter-name (fail-on-found)] reported by reviewdog 🐶
// TODO_IN_THIS_COMMIT: support this flag.
| return nil | ||
| } | ||
|
|
||
| // TODO_IN_THIS_COMMIT: 2-step check |
There was a problem hiding this comment.
[linter-name (fail-on-found)] reported by reviewdog 🐶
// TODO_IN_THIS_COMMIT: 2-step check
| //fmt.Printf(">>> filenames:\n%s\n", strings.Join(filenames, "\n")) | ||
|
|
||
| // TODO_IN_THIS_COMMIT: extract --- BEGIN | ||
| // TODO_IN_THIS_COMMIT: check the filename and only inspect each once! |
There was a problem hiding this comment.
[linter-name (fail-on-found)] reported by reviewdog 🐶
// TODO_IN_THIS_COMMIT: check the filename and only inspect each once!
| continue | ||
| } | ||
|
|
||
| // TODO_IN_THIS_COMMIT: remove! |
There was a problem hiding this comment.
[linter-name (fail-on-found)] reported by reviewdog 🐶
// TODO_IN_THIS_COMMIT: remove!
| return false | ||
| } | ||
|
|
||
| // TODO_IN_THIS_COMMIT: figure out why this file hangs the command. |
There was a problem hiding this comment.
[linter-name (fail-on-found)] reported by reviewdog 🐶
// TODO_IN_THIS_COMMIT: figure out why this file hangs the command.
| ) | ||
| } | ||
|
|
||
| // TODO_IN_THIS_COMMIT: move & godoc... |
There was a problem hiding this comment.
[linter-name (fail-on-found)] reported by reviewdog 🐶
// TODO_IN_THIS_COMMIT: move & godoc...
| @@ -50,3 +50,11 @@ func WithSetupFn(fn func(logger *zerolog.Logger)) polylog.LoggerOption { | |||
| fn(&logger.(*zerologLogger).Logger) | |||
| } | |||
| } | |||
|
|
|||
| // TODO_IN_THIS_COMMIT: godoc & test... | |||
There was a problem hiding this comment.
[linter-name (fail-on-found)] reported by reviewdog 🐶
// TODO_IN_THIS_COMMIT: godoc & test...
| msg, | ||
| strings.Join(offendingPkgErrLines, "\n"), | ||
| msg, | ||
| ) |
There was a problem hiding this comment.
// TODO_IN_THIS_COMMIT: add, set, & return non-zero exit code.
| func runStatusErrorsCheck(cmd *cobra.Command, _ []string) error { | ||
| ctx := cmd.Context() | ||
|
|
||
| // TODO_IN_THIS_COMMIT: extract to validation function. |
There was a problem hiding this comment.
[linter-name (fail-on-found)] reported by reviewdog 🐶
// TODO_IN_THIS_COMMIT: extract to validation function.
|
|
||
| var TRACE bool | ||
|
|
||
| // TODO_IN_THIS_COMMIT: move & godoc... |
There was a problem hiding this comment.
[linter-name (fail-on-found)] reported by reviewdog 🐶
// TODO_IN_THIS_COMMIT: move & godoc...
| // "inspectPosition": pkg.Fset.Position(lastReturnArgNode.Pos()).String(), | ||
| //}).Msg("traversing ast node") | ||
|
|
||
| // TODO_IN_THIS_COMMIT: No need to check that the last return |
There was a problem hiding this comment.
[linter-name (fail-on-found)] reported by reviewdog 🐶
// TODO_IN_THIS_COMMIT: No need to check that the last return
| } | ||
| } | ||
|
|
||
| // TODO_IN_THIS_COMMIT: refactor; below happens when the selector is not found within any package. |
There was a problem hiding this comment.
[linter-name (fail-on-found)] reported by reviewdog 🐶
// TODO_IN_THIS_COMMIT: refactor; below happens when the selector is not found within any package.
| case *ast.Ident: | ||
| logger.Debug().Str("name", expr.Name).Msg("tracing ident") | ||
| //def := candidatePkg.TypesInfo.Defs[expr] | ||
| // TODO_IN_THIS_COMMIT: handle no def... |
There was a problem hiding this comment.
[linter-name (fail-on-found)] reported by reviewdog 🐶
// TODO_IN_THIS_COMMIT: handle no def...
| //def := candidatePkg.TypesInfo.Defs[expr] | ||
| // TODO_IN_THIS_COMMIT: handle no def... | ||
| //x := def.Parent().Lookup(expr.Name) | ||
| // TODO_IN_THIS_COMMIT: handle no lookup... |
There was a problem hiding this comment.
[linter-name (fail-on-found)] reported by reviewdog 🐶
// TODO_IN_THIS_COMMIT: handle no lookup...
| // traceExpressionStack(doa, pkgs, expr, candidatePkg, candidateNode, offendingPositions) | ||
| //case *ast.AssignStmt: | ||
| // logger.Debug().Msgf(">>>>>>> assign stmt: %+v", doa) | ||
| // // TODO_IN_THIS_COMMIT: what about len(Rhs) > 1? |
There was a problem hiding this comment.
[linter-name (fail-on-found)] reported by reviewdog 🐶
// // TODO_IN_THIS_COMMIT: what about len(Rhs) > 1?
| logger.Debug().Msgf("no declaration or assignment found for ident %q", targetIdent.String()) | ||
| } | ||
|
|
||
| // TODO_IN_THIS_COMMIT: improve comment... |
There was a problem hiding this comment.
[linter-name (fail-on-found)] reported by reviewdog 🐶
// TODO_IN_THIS_COMMIT: improve comment...
| } | ||
|
|
||
| if len(assignsRhs) > 0 { | ||
| // TODO_IN_THIS_COMMIT: comment explaining what's going on here... |
There was a problem hiding this comment.
[linter-name (fail-on-found)] reported by reviewdog 🐶
// TODO_IN_THIS_COMMIT: comment explaining what's going on here...
| return declNode, declPos | ||
| } | ||
|
|
||
| // TODO_IN_THIS_COMMIT: move & godoc... |
There was a problem hiding this comment.
[linter-name (fail-on-found)] reported by reviewdog 🐶
// TODO_IN_THIS_COMMIT: move & godoc...
| return declOrAssign, &foundPos | ||
| } | ||
|
|
||
| // TODO_IN_THIS_COMMIT: move & godoc... |
There was a problem hiding this comment.
[linter-name (fail-on-found)] reported by reviewdog 🐶
// TODO_IN_THIS_COMMIT: move & godoc...
| } | ||
| } | ||
|
|
||
| // TODO_IN_THIS_COMMIT: refactor... |
There was a problem hiding this comment.
[linter-name (fail-on-found)] reported by reviewdog 🐶
// TODO_IN_THIS_COMMIT: refactor...
| "github.com/pokt-network/poktroll/pkg/polylog" | ||
| ) | ||
|
|
||
| // TODO_IN_THIS_COMMIT: move & godoc... |
There was a problem hiding this comment.
[linter-name (fail-on-found)] reported by reviewdog 🐶
// TODO_IN_THIS_COMMIT: move & godoc...
|
|
||
| const grpcStatusImportPath = "google.golang.org/grpc/status" | ||
|
|
||
| // TODO_IN_THIS_COMMIT: move & godoc... |
There was a problem hiding this comment.
[linter-name (fail-on-found)] reported by reviewdog 🐶
// TODO_IN_THIS_COMMIT: move & godoc...
| const grpcStatusImportPath = "google.golang.org/grpc/status" | ||
|
|
||
| // TODO_IN_THIS_COMMIT: move & godoc... | ||
| // TODO_IN_THIS_COMMIT: detemine whether this actually needs to return anything. |
There was a problem hiding this comment.
[linter-name (fail-on-found)] reported by reviewdog 🐶
// TODO_IN_THIS_COMMIT: detemine whether this actually needs to return anything.
| logger.Debug().Send() | ||
|
|
||
| // Search for the selector expression in all module packages. | ||
| // TODO_IN_THIS_COMMIT: is it expected and/or guaranteed that it will only be found in one pkg? |
There was a problem hiding this comment.
[linter-name (fail-on-found)] reported by reviewdog 🐶
// TODO_IN_THIS_COMMIT: is it expected and/or guaranteed that it will only be found in one pkg?
| } | ||
| } | ||
|
|
||
| // TODO_IN_THIS_COMMIT: note early return... |
There was a problem hiding this comment.
[linter-name (fail-on-found)] reported by reviewdog 🐶
// TODO_IN_THIS_COMMIT: note early return...
| return false | ||
| } | ||
|
|
||
| // TODO_IN_THIS_COMMIT: move & godoc... |
There was a problem hiding this comment.
[linter-name (fail-on-found)] reported by reviewdog 🐶
// TODO_IN_THIS_COMMIT: move & godoc...
| return nil | ||
| } | ||
|
|
||
| // TODO_IN_THIS_COMMIT: move & godoc... |
There was a problem hiding this comment.
[linter-name (fail-on-found)] reported by reviewdog 🐶
// TODO_IN_THIS_COMMIT: move & godoc...
| } | ||
| } | ||
|
|
||
| // TODO_IN_THIS_COMMIT: move & godoc... |
There was a problem hiding this comment.
[linter-name (fail-on-found)] reported by reviewdog 🐶
// TODO_IN_THIS_COMMIT: move & godoc...
| offendingPkgErrLineSet[sourceLine] = struct{}{} | ||
| } | ||
|
|
||
| // TODO_IN_THIS_COMMIT: move & godoc... |
There was a problem hiding this comment.
[linter-name (fail-on-found)] reported by reviewdog 🐶
// TODO_IN_THIS_COMMIT: move & godoc...
| } | ||
| } | ||
|
|
||
| // TODO_IN_THIS_COMMIT: move & godoc... exits with code CodeNonStatusGRPCErrorsFound if offending lines found if offending lines found. |
There was a problem hiding this comment.
[linter-name (fail-on-found)] reported by reviewdog 🐶
// TODO_IN_THIS_COMMIT: move & godoc... exits with code CodeNonStatusGRPCErrorsFound if offending lines found if offending lines found.
bryanchriswhite
force-pushed
the
issues/860/chore/grpc-errors
branch
from
2702b60 to
3bbb3c9
Compare
bryanchriswhite
force-pushed
the
issues/860/chore/grpc-errors
branch
from
3bbb3c9 to
4169627
Compare
| RunE: runStatusErrorsCheck, | ||
| } | ||
|
|
||
| // TODO_IN_THIS_COMMIT: refactor to avoid global logger var. |
There was a problem hiding this comment.
[linter-name (fail-on-found)] reported by reviewdog 🐶
// TODO_IN_THIS_COMMIT: refactor to avoid global logger var.
| Tests: false, | ||
| } | ||
|
|
||
| // TODO_IN_THIS_COMMIT: update comment... |
There was a problem hiding this comment.
[linter-name (fail-on-found)] reported by reviewdog 🐶
// TODO_IN_THIS_COMMIT: update comment...
| return nil | ||
| } | ||
|
|
||
| // TODO_IN_THIS_COMMIT: move & godoc... |
There was a problem hiding this comment.
[linter-name (fail-on-found)] reported by reviewdog 🐶
// TODO_IN_THIS_COMMIT: move & godoc...
| return nil | ||
| } | ||
|
|
||
| // TODO_IN_THIS_COMMIT: move & godoc... |
There was a problem hiding this comment.
[linter-name (fail-on-found)] reported by reviewdog 🐶
// TODO_IN_THIS_COMMIT: move & godoc...
| return false | ||
| } | ||
|
|
||
| // TODO_IN_THIS_COMMIT: move & godoc... |
There was a problem hiding this comment.
[linter-name (fail-on-found)] reported by reviewdog 🐶
// TODO_IN_THIS_COMMIT: move & godoc...
| return nil | ||
| } | ||
|
|
||
| // TODO_IN_THIS_COMMIT: move & godoc... |
There was a problem hiding this comment.
[linter-name (fail-on-found)] reported by reviewdog 🐶
// TODO_IN_THIS_COMMIT: move & godoc...
| } | ||
| } | ||
|
|
||
| // TODO_IN_THIS_COMMIT: move & godoc... |
There was a problem hiding this comment.
[linter-name (fail-on-found)] reported by reviewdog 🐶
// TODO_IN_THIS_COMMIT: move & godoc...
| offendingPkgErrLineSet[sourceLine] = struct{}{} | ||
| } | ||
|
|
||
| // TODO_IN_THIS_COMMIT: move & godoc... |
There was a problem hiding this comment.
[linter-name (fail-on-found)] reported by reviewdog 🐶
// TODO_IN_THIS_COMMIT: move & godoc...
| } | ||
| } | ||
|
|
||
| // TODO_IN_THIS_COMMIT: move & godoc... exits with code CodeNonStatusGRPCErrorsFound if offending lines found if offending lines found. |
There was a problem hiding this comment.
[linter-name (fail-on-found)] reported by reviewdog 🐶
// TODO_IN_THIS_COMMIT: move & godoc... exits with code CodeNonStatusGRPCErrorsFound if offending lines found if offending lines found.
Olshansk
modified the milestones:
Shannon Beta TestNet Launch,
Shannon Beta TestNet Support
Summary
Adds a
status-errorssubcommand toprotocheckwhich identifiesmsgServermethods and query handlers which don't return gRPC status error codes:Issue
Type of change
Select one or more from the following:
consensus-breakinglabel if so. See [Infra] Automatically add theconsensus-breakinglabel #791 for detailsTesting
make docusaurus_start; only needed if you make doc changesmake go_develop_and_testmake test_e2edevnet-test-e2elabel to the PR.Sanity Checklist