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

feat(version): Add extraInfo to version cmd #18063

Merged
merged 15 commits into from
Oct 30, 2023
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,7 @@ Ref: https://keepachangelog.com/en/1.0.0/
* (rpc) [#17470](https://github.com/cosmos/cosmos-sdk/pull/17470) Avoid open 0.0.0.0 to public by default and add `listen-ip-address` argument for `testnet init-files` cmd.
* (types) [#17670](https://github.com/cosmos/cosmos-sdk/pull/17670) Use `ctx.CometInfo` in place of `ctx.VoteInfos`
* [#17733](https://github.com/cosmos/cosmos-sdk/pull/17733) Ensure `buf export` exports all proto dependencies
* (version) [#18063](https://github.com/cosmos/cosmos-sdk/pull/18063) Include additional information in the Info struct. This change enhances the Info struct by adding support for additional information through the ExtraInfo field
* [#18204](https://github.com/cosmos/cosmos-sdk/pull/18204) Use streaming json parser to parse chain-id from genesis file.

### Bug Fixes
Expand Down
20 changes: 20 additions & 0 deletions version/command.go
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,11 @@ const (
)

// NewVersionCommand returns a CLI command to interactively print the application binary version information.
// Note: When seeking to add the extra info to the context
// The below can be added to the initRootCmd to include the extraInfo field
//
// cmdContext := context.WithValue(context.Background(), version.ContextKey{}, extraInfo)
// rootCmd.SetContext(cmdContext)
Comment on lines +18 to +22
Copy link
Contributor

Choose a reason for hiding this comment

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

The comment provides instructions on how to add extra info to the context. However, it would be more maintainable and less error-prone if this was handled by the function itself, rather than requiring the caller to do it. Consider refactoring NewVersionCommand to accept an ExtraInfo parameter, which it can then add to the context.

func NewVersionCommand() *cobra.Command {
cmd := &cobra.Command{
Use: "version",
Expand All @@ -28,6 +33,10 @@ func NewVersionCommand() *cobra.Command {
return nil
}

// Extract and set extra information from the context
extraInfo := extraInfoFromContext(cmd)
verInfo.ExtraInfo = &extraInfo
Comment on lines +36 to +38
Copy link
Contributor

Choose a reason for hiding this comment

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

The extraInfoFromContext function is used to extract extra information from the context. However, there is no error handling if the context does not contain ExtraInfo. This could lead to unexpected behavior if the context is not properly initialized. Consider adding error handling or a default value for ExtraInfo.


var (
bz []byte
err error
Expand Down Expand Up @@ -56,3 +65,14 @@ func NewVersionCommand() *cobra.Command {

return cmd
}

func extraInfoFromContext(cmd *cobra.Command) ExtraInfo {
ctx := cmd.Context()
if ctx != nil {
extraInfo, ok := ctx.Value(ContextKey{}).(ExtraInfo)
if ok {
return extraInfo
}
}
return nil
}
samricotta marked this conversation as resolved.
Show resolved Hide resolved
7 changes: 7 additions & 0 deletions version/version.go
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,9 @@ import (
"runtime/debug"
)

// ContextKey is used to store the ExtraInfo in the context.
type ContextKey struct{}

var (
// application's name
Name = ""
Expand Down Expand Up @@ -55,6 +58,9 @@ func getSDKVersion() string {
return sdkVersion
}

// ExtraInfo contains a set of extra information provided by apps
type ExtraInfo map[string]string

Comment on lines +61 to +63
Copy link
Contributor

Choose a reason for hiding this comment

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

The ExtraInfo type is defined as a map of string to string. This is a flexible structure that can hold various types of information. However, it might be worth considering if a more structured approach would be beneficial, especially if there are specific pieces of information that will always be included in the ExtraInfo. If that's the case, a struct with specific fields might be a better choice.

// Info defines the application version information.
type Info struct {
Name string `json:"name" yaml:"name"`
Expand All @@ -65,6 +71,7 @@ type Info struct {
GoVersion string `json:"go" yaml:"go"`
BuildDeps []buildDep `json:"build_deps" yaml:"build_deps"`
CosmosSdkVersion string `json:"cosmos_sdk_version" yaml:"cosmos_sdk_version"`
ExtraInfo *ExtraInfo `json:"extra_info" yaml:"extra_info"`
}

func NewInfo() Info {
Expand Down
20 changes: 17 additions & 3 deletions version/version_test.go
Original file line number Diff line number Diff line change
@@ -1,13 +1,13 @@
package version_test

import (
context "context"
Copy link
Contributor

Choose a reason for hiding this comment

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

The import of the context package is redundant as it's already included in the standard library and doesn't need to be aliased. You can directly use context.Background() or context.WithValue() without importing it explicitly.

- context "context"

Commitable suggestion (Beta)
Suggested change
context "context"
# context package is already included in the standard library
# You can directly use context.Background() or context.WithValue() without importing it explicitly.

"encoding/json"
"fmt"
"runtime"
"strings"
"testing"

"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"

"github.com/cosmos/cosmos-sdk/client/flags"
Expand Down Expand Up @@ -149,7 +149,7 @@ func Test_runVersionCmd(t *testing.T) {
})

require.NoError(t, cmd.Execute())
assert.Equal(t, "\n", mockOut.String())
require.Equal(t, "\n", mockOut.String())
mockOut.Reset()

cmd.SetArgs([]string{
Expand All @@ -158,7 +158,21 @@ func Test_runVersionCmd(t *testing.T) {

info := version.NewInfo()
stringInfo, err := json.Marshal(info)

extraInfo := &version.ExtraInfo{"key1": "value1"}
ctx := context.WithValue(context.Background(), version.ContextKey{}, extraInfo)
tac0turtle marked this conversation as resolved.
Show resolved Hide resolved

require.NoError(t, err)
require.NoError(t, cmd.Execute())
assert.Equal(t, string(stringInfo)+"\n", mockOut.String())

extraInfoFromContext := ctx.Value(version.ContextKey{})
require.NotNil(t, extraInfoFromContext)

castedExtraInfo, ok := extraInfoFromContext.(*version.ExtraInfo)
require.True(t, ok)

key1Value := (*castedExtraInfo)["key1"]
require.Equal(t, "value1", key1Value)

require.Equal(t, string(stringInfo)+"\n", mockOut.String())
}
julienrbrt marked this conversation as resolved.
Show resolved Hide resolved