-
Notifications
You must be signed in to change notification settings - Fork 80
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
cmd: add alpha view-cluster-manifest
command
#2408
Conversation
Moved some manifest load/store functions to `manifest_tools.go` and generalized them, so we can reuse code across multiple commands.
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #2408 +/- ##
==========================================
+ Coverage 53.67% 53.77% +0.10%
==========================================
Files 196 198 +2
Lines 26326 26418 +92
==========================================
+ Hits 14130 14207 +77
- Misses 10441 10450 +9
- Partials 1755 1761 +6
☔ View full report in Codecov by Sentry. |
cmd/view_cluster_manifest.go
Outdated
// if we're able to unmarshal in a map do it, otherwise hex encode and return | ||
brJSON := make(map[string]any) | ||
if err := json.Unmarshal(bytes, &brJSON); err != nil { | ||
return "0x" + hex.EncodeToString(bytes) | ||
} | ||
|
||
return brJSON |
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.
nit: can replace with: return fmt.Sprint("%#x", bytes)
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.
oh, maybe this supports marshalling nested json fields
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.
if bytes contains valid json we return it unmarshaled in a map, otherwise its hex encoded representation - this code is here to give users plaintext validator registrations instead of a escaped string containing raw JSON
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 did replace hex.EncodeToString with the sprintf suggestion though
@@ -46,6 +46,7 @@ func New() *cobra.Command { | |||
newCombineCmd(newCombineFunc), | |||
newAlphaCmd( | |||
newAddValidatorsCmd(runAddValidatorsSolo), | |||
newViewClusterManifestCmd(runViewClusterManifest), |
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.
nit: fix indent if broken
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.
wdym?
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.
Nvm, when viewing the PR, i couldn't see a tab space
cmd/view_cluster_manifest.go
Outdated
return viewClusterManifest(manifestFilePath, out) | ||
} | ||
|
||
func viewClusterManifest(manifestFilePath string, out io.Writer) error { |
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.
nit: add godoc
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 removed that function and replaced it with a plain runViewClusterManifest
- I don't think this function needs godoc (isn't exported, meaning of the function is clear enough).
This struct doesn't need to be exported, and is also only used in add-validator tests.
Moved some manifest load/store functions to
manifest_tools.go
and generalized them, so we can reuse code across multiple commands.Closes #2381
category: feature
ticket: #2381