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): update version command to return v2 info #22807

Merged
merged 15 commits into from
Dec 10, 2024

Conversation

aljo242
Copy link
Contributor

@aljo242 aljo242 commented Dec 9, 2024

Description

Closes: #22772

  • add versions to the query
  • refactor the Info() fuction so internals can be tested

Try it out

COSMOS_BUILD_OPTIONS=v2 make install
simdv2 version --long

Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

  • included the correct type prefix in the PR title, you can find examples of the prefixes below:
  • confirmed ! in the type prefix if API or client breaking change
  • targeted the correct branch (see PR Targeting)
  • provided a link to the relevant issue or specification
  • reviewed "Files changed" and left comments if necessary
  • included the necessary unit and integration tests
  • added a changelog entry to CHANGELOG.md
  • updated the relevant documentation or specification, including comments for documenting Go code
  • confirmed all CI checks have passed

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.

Please see Pull Request Reviewer section in the contributing guide for more information on how to review a pull request.

I have...

  • confirmed the correct type prefix in the PR title
  • confirmed all author checklist items have been addressed
  • reviewed state machine logic, API design and naming, documentation is accurate, tests and test coverage

Summary by CodeRabbit

  • New Features

    • Added support for returning v2 server information in the version command.
    • Enhanced GetNodeInfo response with additional version fields: RuntimeVersion, CometServerVersion, and StfVersion.
  • Improvements

    • Changelog format updated for better readability and organization.
    • Enhanced error handling for block height requests.
    • Updated versioning information structure for improved tracking of dependencies.
  • Bug Fixes

    • Improved response structure for validator set queries.
  • Tests

    • Introduced unit tests for SDK build information retrieval functions.
  • Documentation

    • Updated changelog to reflect recent changes and enhancements.

@aljo242 aljo242 requested review from julienrbrt, JulianToledano and a team as code owners December 9, 2024 17:47
Copy link
Contributor

coderabbitai bot commented Dec 9, 2024

📝 Walkthrough
📝 Walkthrough

Walkthrough

The pull request introduces several enhancements across multiple files, primarily focusing on the version command to return detailed server information, including new fields in various response structures. The CHANGELOG.md has been updated to reflect these changes, clearly delineating version updates. The cmtservice package has improved response data, while the proto file has been modified to include additional versioning information. Unit tests for SDK build information have also been added, ensuring robust validation of the new features.

Changes

File Change Summary
CHANGELOG.md Added entry for v2 server information in the version command; structured changelog for clarity.
client/grpc/cmtservice/service.go Updated GetNodeInfo to include new fields; refined error handling in GetBlockByHeight.
proto/cosmos/base/tendermint/v1beta1/query.proto Expanded VersionInfo message with new fields for detailed versioning information.
version/build_test.go Added unit tests for SDK build information extraction functions.
version/version.go Replaced getSDKVersion with getSDKBuildInfo; updated Info struct with new version fields.
version/version_test.go Modified imports and updated Info struct in tests to include new CosmosSdkVersion field.

Assessment against linked issues

Objective Addressed Explanation
Update version command to display server/v2 info (#[22772])
Add runtime/v2, stf, and server/v2/cometbft versions to long description (#[22772])

Possibly related PRs

Suggested labels

C:server/v2, C:x/genutil

Suggested reviewers

  • julienrbrt
  • akhilkumarpilli
  • kocubinski
  • facundomedica

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR. (Experiment)
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

repeated Module build_deps = 7;
string cosmos_sdk_version = 8 [(cosmos_proto.field_added_in) = "cosmos-sdk 0.43"];
string comet_server_version = 9 [(cosmos_proto.field_added_in) = "cosmos-sdk 2.0"];
string runtime_version = 10 [(cosmos_proto.field_added_in) = "cosmos-sdk 2.0"];
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is this the proper version to be adding as an annotation here? not sure

Copy link
Member

Choose a reason for hiding this comment

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

It should be cosmos-sdk 0.52, as this is part of the cosmos/cosmos-sdk go.mod

@github-actions github-actions bot added the C:CLI label Dec 9, 2024
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🧹 Outside diff range and nitpick comments (7)
api/cosmos/base/tendermint/v1beta1/query.pulsar.go (1)

11731-11743: Consider adding version format validation

The version fields are currently stored as plain strings. Consider adding validation to ensure they follow semantic versioning format or your project's version format conventions.

Example validation implementation:

+func validateVersion(version string) error {
+    // Add your version format validation logic here
+    if !regexp.MatchString(`^v?\d+\.\d+\.\d+`, version) {
+        return fmt.Errorf("invalid version format: %s", version)
+    }
+    return nil
+}
version/build_test.go (1)

119-119: Format code with gofumpt for consistency

The file is not formatted according to gofumpt standards. Please run gofumpt -w -extra build_test.go to ensure consistent code formatting across the project.

This helps maintain code readability and adherence to the project's style guidelines.

🧰 Tools
🪛 golangci-lint (1.62.2)

119-119: File is not gofumpt-ed with -extra

(gofumpt)

version/version_test.go (1)

15-15: Organize imports using gci for proper import grouping

The import statements are not ordered according to the project's conventions. Please use gci with the provided configuration to reorder the imports:

gci write --skip-generated -s standard -s default -s prefix(cosmossdk.io) -s prefix(github.com/cosmos/cosmos-sdk) --custom-order version_test.go

This ensures consistency in how imports are organized throughout the codebase.

🧰 Tools
🪛 golangci-lint (1.62.2)

15-15: File is not gci-ed with --skip-generated -s standard -s default -s prefix(cosmossdk.io) -s prefix(github.com/cosmos/cosmos-sdk) --custom-order

(gci)

client/grpc/cmtservice/service.go (1)

230-240: LGTM! Consider adding documentation and validation.

The implementation correctly adds the new version fields as per the PR objectives. However, consider these improvements:

  1. Add field documentation to explain the purpose and format of the new version fields:
 ApplicationVersion: &VersionInfo{
+    // AppName is the application name
     AppName:            nodeInfo.AppName,
+    // RuntimeVersion represents the version of the runtime/v2 dependency
     RuntimeVersion:     nodeInfo.RuntimeVersion,
+    // CometServerVersion represents the version of server/v2/cometbft
     CometServerVersion: nodeInfo.CometServerVersion,
+    // StfVersion represents the version of the stf dependency
     StfVersion:         nodeInfo.StfVersion,
     // ... other fields
 }
  1. Consider adding validation or default values for the new version fields:
 ApplicationVersion: &VersionInfo{
     // ... other fields
-    RuntimeVersion:     nodeInfo.RuntimeVersion,
-    CometServerVersion: nodeInfo.CometServerVersion,
-    StfVersion:         nodeInfo.StfVersion,
+    RuntimeVersion:     getVersionOrDefault(nodeInfo.RuntimeVersion),
+    CometServerVersion: getVersionOrDefault(nodeInfo.CometServerVersion),
+    StfVersion:         getVersionOrDefault(nodeInfo.StfVersion),
 }

Add this helper function:

// getVersionOrDefault returns the version if non-empty or "not available"
func getVersionOrDefault(version string) string {
    if version == "" {
        return "not available"
    }
    return version
}
CHANGELOG.md (3)

48-48: Fix inconsistent formatting in changelog entry

The changelog entry for client improvements is not properly indented and aligned with other entries in the same section.

-* (client) [#22807](https://github.com/cosmos/cosmos-sdk/pull/22807) Return v2 server information in the `version` command.
+* (client) [#22807](https://github.com/cosmos/cosmos-sdk/pull/22807) Return v2 server information in the `version` command.

Line range hint 785-786: Improve clarity of breaking change description

The description of the breaking change is unclear and could be expanded to better explain the impact.

-* (x/auth) [#13780](https://github.com/cosmos/cosmos-sdk/pull/13780) `id` (type of int64) in `AccountAddressByID` grpc query is now deprecated, update to account-id(type of uint64) to use `AccountAddressByID`.
+* (x/auth) [#13780](https://github.com/cosmos/cosmos-sdk/pull/13780) The `id` field (type int64) in `AccountAddressByID` gRPC query is now deprecated. Use account-id (type uint64) instead when calling `AccountAddressByID`.

Line range hint 1401-1404: Fix bullet point formatting inconsistency

The bullet points under logging improvements are not consistently formatted.

-_ Use [zerolog](https://github.com/rs/zerolog) over Tendermint's go-kit logging wrapper.
-_ Introduce Tendermint's `--log_format=plain|json` flag. Using format `json` allows for emitting structured JSON
-logs which can be consumed by an external logging facility (e.g. Loggly). Both formats log to STDERR. \* The existing `--log_level` flag and it's default value now solely relates to the global logging
-level (e.g. `info`, `debug`, etc...) instead of `<module>:<level>`.
+* Use [zerolog](https://github.com/rs/zerolog) over Tendermint's go-kit logging wrapper.
+* Introduce Tendermint's `--log_format=plain|json` flag. Using format `json` allows for emitting structured JSON
+  logs which can be consumed by an external logging facility (e.g. Loggly). Both formats log to STDERR.
+* The existing `--log_level` flag and its default value now solely relates to the global logging
+  level (e.g. `info`, `debug`, etc...) instead of `<module>:<level>`.
📜 Review details

Configuration used: .coderabbit.yml
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 34f407d and b3d6e97.

⛔ Files ignored due to path filters (1)
  • client/grpc/cmtservice/query.pb.go is excluded by !**/*.pb.go
📒 Files selected for processing (7)
  • CHANGELOG.md (1 hunks)
  • api/cosmos/base/tendermint/v1beta1/query.pulsar.go (16 hunks)
  • client/grpc/cmtservice/service.go (1 hunks)
  • proto/cosmos/base/tendermint/v1beta1/query.proto (1 hunks)
  • version/build_test.go (1 hunks)
  • version/version.go (2 hunks)
  • version/version_test.go (2 hunks)
🔥 Files not summarized due to errors (1)
  • api/cosmos/base/tendermint/v1beta1/query.pulsar.go: Error: Server error: no LLM provider could handle the message
🧰 Additional context used
📓 Path-based instructions (6)
api/cosmos/base/tendermint/v1beta1/query.pulsar.go (1)

Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.

version/version_test.go (2)

Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.


Pattern **/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"

version/build_test.go (2)

Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.


Pattern **/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"

client/grpc/cmtservice/service.go (1)

Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.

version/version.go (1)

Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.

CHANGELOG.md (1)

Pattern **/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"

🪛 golangci-lint (1.62.2)
version/version_test.go

15-15: File is not gci-ed with --skip-generated -s standard -s default -s prefix(cosmossdk.io) -s prefix(github.com/cosmos/cosmos-sdk) --custom-order

(gci)

version/build_test.go

119-119: File is not gofumpt-ed with -extra

(gofumpt)

🔇 Additional comments (11)
api/cosmos/base/tendermint/v1beta1/query.pulsar.go (4)

6413-6424: LGTM: Field descriptors properly defined

The new field descriptors for version information are correctly defined following the existing pattern.


6438-6440: LGTM: Field initialization is correct

The new version fields are properly initialized in the init() function, maintaining consistency with existing field initialization patterns.


11242-11261: LGTM: Getter methods properly implemented

The getter methods for the new version fields are correctly implemented with proper null checks and default values.


11153-11163: Consider version compatibility implications

The new version fields (comet_server_version, runtime_version, stf_version) are annotated with cosmos-sdk 2.0 constraints. Ensure that this version requirement is documented and that proper fallback behavior exists for older versions.

version/version.go (4)

42-47: Definition of sdkBuildInfo struct is appropriate

The new sdkBuildInfo struct effectively encapsulates the SDK and related version information needed for enhanced version reporting.


49-65: Implementation of getSDKBuildInfo function correctly extracts version information

The getSDKBuildInfo function accurately parses the build dependencies to retrieve the required version details for each component.


67-72: extractVersionFromBuildInfo handles module replacements properly

The function accounts for module replacements, ensuring that the correct version is returned, whether or not a replacement is in place.


80-91: Addition of new fields to Info struct enriches version data

Including RuntimeVersion, StfVersion, and CometServerVersion in the Info struct enhances the granularity of version information available to users.

version/build_test.go (1)

1-166: Unit tests provide adequate coverage for new functionality

The tests in build_test.go effectively cover the new functions getSDKBuildInfo, extractVersionFromBuildInfo, and depsFromBuildInfo, ensuring they behave as expected under various scenarios.

🧰 Tools
🪛 golangci-lint (1.62.2)

119-119: File is not gofumpt-ed with -extra

(gofumpt)

version/version_test.go (1)

88-93: Update test cases to reflect changes in Info struct

The addition of CosmosSdkVersion in the Info struct necessitates updating the expected values in the test cases to ensure accurate validation.

The test adjustments correctly account for the new field, maintaining the integrity of the unit tests.

proto/cosmos/base/tendermint/v1beta1/query.proto (1)

155-157: Verify the field_added_in annotations for accuracy

Please confirm that the [(cosmos_proto.field_added_in) = "cosmos-sdk 2.0"] annotations for the fields comet_server_version, runtime_version, and stf_version correctly reflect the version in which these fields are added. Is "cosmos-sdk 2.0" the appropriate version identifier?

This ensures that documentation and tooling relying on these annotations are accurate.

Comment on lines 95 to 116
info := Info{
Name: Name,
AppName: AppName,
Version: Version,
GitCommit: Commit,
BuildTags: BuildTags,
GoVersion: fmt.Sprintf("go version %s %s/%s", runtime.Version(), runtime.GOOS, runtime.GOARCH),
BuildDeps: depsFromBuildInfo(),
CosmosSdkVersion: sdkVersion,
CosmosSdkVersion: "unable to read deps",
}

// use debug info more granular build info if available
debugBuildInfo, ok := debug.ReadBuildInfo()
if ok {
info.BuildDeps = depsFromBuildInfo(debugBuildInfo)
sdkBuildInfo := getSDKBuildInfo(debugBuildInfo)
info.CometServerVersion = sdkBuildInfo.sdkVersion
info.RuntimeVersion = sdkBuildInfo.runtimeVersion
info.StfVersion = sdkBuildInfo.stfVersion
info.CometServerVersion = sdkBuildInfo.cometServerVersion
}

return info
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Incorrect assignments in NewInfo() function

There is an error in the NewInfo() function where info.CometServerVersion is assigned the value of sdkBuildInfo.sdkVersion instead of sdkBuildInfo.cometServerVersion. Additionally, info.CosmosSdkVersion is not updated with the actual SDK version from the build info.

Apply the following diff to correct the assignments:

     if ok {
         info.BuildDeps = depsFromBuildInfo(debugBuildInfo)
         sdkBuildInfo := getSDKBuildInfo(debugBuildInfo)
-        info.CometServerVersion = sdkBuildInfo.sdkVersion
+        info.CosmosSdkVersion = sdkBuildInfo.sdkVersion
         info.RuntimeVersion = sdkBuildInfo.runtimeVersion
         info.StfVersion = sdkBuildInfo.stfVersion
         info.CometServerVersion = sdkBuildInfo.cometServerVersion
     }

This ensures that info.CosmosSdkVersion correctly reflects the SDK version, and info.CometServerVersion is assigned the appropriate value.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
info := Info{
Name: Name,
AppName: AppName,
Version: Version,
GitCommit: Commit,
BuildTags: BuildTags,
GoVersion: fmt.Sprintf("go version %s %s/%s", runtime.Version(), runtime.GOOS, runtime.GOARCH),
BuildDeps: depsFromBuildInfo(),
CosmosSdkVersion: sdkVersion,
CosmosSdkVersion: "unable to read deps",
}
// use debug info more granular build info if available
debugBuildInfo, ok := debug.ReadBuildInfo()
if ok {
info.BuildDeps = depsFromBuildInfo(debugBuildInfo)
sdkBuildInfo := getSDKBuildInfo(debugBuildInfo)
info.CometServerVersion = sdkBuildInfo.sdkVersion
info.RuntimeVersion = sdkBuildInfo.runtimeVersion
info.StfVersion = sdkBuildInfo.stfVersion
info.CometServerVersion = sdkBuildInfo.cometServerVersion
}
return info
info := Info{
Name: Name,
AppName: AppName,
Version: Version,
GitCommit: Commit,
BuildTags: BuildTags,
GoVersion: fmt.Sprintf("go version %s %s/%s", runtime.Version(), runtime.GOOS, runtime.GOARCH),
CosmosSdkVersion: "unable to read deps",
}
// use debug info more granular build info if available
debugBuildInfo, ok := debug.ReadBuildInfo()
if ok {
info.BuildDeps = depsFromBuildInfo(debugBuildInfo)
sdkBuildInfo := getSDKBuildInfo(debugBuildInfo)
info.CosmosSdkVersion = sdkBuildInfo.sdkVersion
info.RuntimeVersion = sdkBuildInfo.runtimeVersion
info.StfVersion = sdkBuildInfo.stfVersion
info.CometServerVersion = sdkBuildInfo.cometServerVersion
}
return info

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Outside diff range and nitpick comments (1)
version/version.go (1)

49-72: Consider enhancing robustness and maintainability

While the implementation is correct, consider these improvements:

  1. Add error handling for edge cases where version information might be missing
  2. Define package paths as constants to avoid string literals and make maintenance easier
+const (
+    cosmosSDKPath       = "github.com/cosmos/cosmos-sdk"
+    cometServerPath     = "cosmossdk.io/server/v2/cometbft"
+    runtimePath         = "cosmossdk.io/runtime/v2"
+    stfPath             = "cosmossdk.io/server/v2/stf"
+)

 func getSDKBuildInfo(debugBuildInfo *debug.BuildInfo) sdkBuildInfo {
     var buildInfo sdkBuildInfo
     for _, dep := range debugBuildInfo.Deps {
         switch dep.Path {
-        case "github.com/cosmos/cosmos-sdk":
+        case cosmosSDKPath:
             buildInfo.sdkVersion = extractVersionFromBuildInfo(dep)
-        case "cosmossdk.io/server/v2/cometbft":
+        case cometServerPath:
             buildInfo.cometServerVersion = extractVersionFromBuildInfo(dep)
-        case "cosmossdk.io/runtime/v2":
+        case runtimePath:
             buildInfo.runtimeVersion = extractVersionFromBuildInfo(dep)
-        case "cosmossdk.io/server/v2/stf":
+        case stfPath:
             buildInfo.stfVersion = extractVersionFromBuildInfo(dep)
         }
     }
📜 Review details

Configuration used: .coderabbit.yml
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between b3d6e97 and 8b88721.

📒 Files selected for processing (1)
  • version/version.go (2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
version/version.go (1)

Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.

🔇 Additional comments (4)
version/version.go (4)

42-47: LGTM: Well-structured version information struct

The sdkBuildInfo struct is well-designed with clear field names that follow Go naming conventions.


80-91: LGTM: Well-defined struct with proper field tags

The Info struct is well-designed with:

  • Clear field names
  • Proper JSON and YAML tags
  • Appropriate use of omitempty for optional fields

95-116: LGTM: Version assignments have been fixed

The implementation correctly assigns version information from the build info, addressing the issues raised in the previous review.


128-132: LGTM: Clean implementation of dependency extraction

The function is simple, focused, and correctly handles the dependency extraction.

repeated Module build_deps = 7;
string cosmos_sdk_version = 8 [(cosmos_proto.field_added_in) = "cosmos-sdk 0.43"];
string comet_server_version = 9 [(cosmos_proto.field_added_in) = "cosmos-sdk 2.0"];
string runtime_version = 10 [(cosmos_proto.field_added_in) = "cosmos-sdk 2.0"];
Copy link
Member

Choose a reason for hiding this comment

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

It should be cosmos-sdk 0.52, as this is part of the cosmos/cosmos-sdk go.mod

"github.com/cosmos/cosmos-sdk/client/flags"
"github.com/cosmos/cosmos-sdk/testutil"
"github.com/cosmos/cosmos-sdk/testutil/cmdtest"
"github.com/cosmos/cosmos-sdk/version"
"github.com/stretchr/testify/require"
Copy link
Member

Choose a reason for hiding this comment

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

Can you run make lint-fix

@@ -45,6 +45,7 @@ Every module contains its own CHANGELOG.md. Please refer to the module you are i
* (baseapp) [#20291](https://github.com/cosmos/cosmos-sdk/pull/20291) Simulate nested messages.
* (crypto/keyring) [#21653](https://github.com/cosmos/cosmos-sdk/pull/21653) New Linux-only backend that adds Linux kernel's `keyctl` support.
* (client/keys) [#21829](https://github.com/cosmos/cosmos-sdk/pull/21829) Add support for importing hex key using standard input.
* (client) [#22807](https://github.com/cosmos/cosmos-sdk/pull/22807) Return v2 server information in the `version` command.
Copy link
Member

Choose a reason for hiding this comment

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

should be (version)

@julienrbrt julienrbrt changed the title feat: update version command to return v2 info feat(version): update version command to return v2 info Dec 9, 2024
@julienrbrt julienrbrt added the backport/v0.52.x PR scheduled for inclusion in the v0.52's next stable release label Dec 9, 2024
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🧹 Outside diff range and nitpick comments (1)
api/cosmos/base/tendermint/v1beta1/query.pulsar.go (1)

Line range hint 1-11883: Generated code follows protobuf-go standards

This is a generated file that correctly implements all required protobuf functionality for the new version fields. Remember not to manually modify this file; instead, update the source .proto files.

📜 Review details

Configuration used: .coderabbit.yml
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 8b88721 and e13146d.

⛔ Files ignored due to path filters (1)
  • client/grpc/cmtservice/query.pb.go is excluded by !**/*.pb.go
📒 Files selected for processing (4)
  • api/cosmos/base/tendermint/v1beta1/query.pulsar.go (16 hunks)
  • proto/cosmos/base/tendermint/v1beta1/query.proto (1 hunks)
  • version/build_test.go (1 hunks)
  • version/version_test.go (1 hunks)
🔥 Files not summarized due to errors (1)
  • api/cosmos/base/tendermint/v1beta1/query.pulsar.go: Error: Server error: no LLM provider could handle the message
🚧 Files skipped from review as they are similar to previous changes (1)
  • version/build_test.go
🧰 Additional context used
📓 Path-based instructions (2)
api/cosmos/base/tendermint/v1beta1/query.pulsar.go (1)

Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.

version/version_test.go (2)

Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.


Pattern **/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"

🔇 Additional comments (6)
proto/cosmos/base/tendermint/v1beta1/query.proto (2)

146-153: LGTM! Clean formatting improvements

The reformatting of the VersionInfo message fields improves readability while maintaining functionality.


154-156: Verify cosmos-sdk version annotation

The new fields are well-structured and align with the PR objectives. However, based on previous review discussions, we should verify the cosmos-sdk version annotation.

Let's verify the cosmos-sdk version in the repository:

✅ Verification successful

Version annotations are consistent with SDK versioning

The new fields (comet_server_version, runtime_version, stf_version) are correctly annotated with cosmos-sdk 0.52. This is consistent with other version annotations in the same file, which follow a similar pattern (e.g., cosmos_sdk_version field is annotated with cosmos-sdk 0.43). The version annotations are properly used to track field additions across SDK releases.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the cosmos-sdk version used in the repository

# Check go.mod for cosmos-sdk version
echo "Checking go.mod for cosmos-sdk version:"
rg "github.com/cosmos/cosmos-sdk" go.mod

# Check if there are any other proto files using version annotations
echo -e "\nChecking other proto files for version annotations:"
rg -A 1 "cosmos_proto.field_added_in.*cosmos-sdk" --type proto

Length of output: 442


Script:

#!/bin/bash
# Let's try a different approach to find proto files and check version annotations

# First, find all proto files
echo "Finding all proto files:"
fd -e proto

# Then check for version annotations in these files
echo -e "\nChecking version annotations in proto files:"
fd -e proto -x grep -l "cosmos_proto.field_added_in" {} \; -x cat {}

# Also check for any version-related configuration files
echo -e "\nChecking for version-related files:"
fd -g "*version*"

Length of output: 114918


Script:

#!/bin/bash
# Let's search for cosmos-sdk version 0.52 annotations in proto files
echo "Searching for cosmos-sdk 0.52 annotations:"
rg -A 1 "cosmos-sdk 0.52" --type proto

# Also check for any other version annotations in the same file
echo -e "\nChecking for other version annotations in query.proto:"
cat proto/cosmos/base/tendermint/v1beta1/query.proto

Length of output: 8567

api/cosmos/base/tendermint/v1beta1/query.pulsar.go (4)

6413-6424: Field descriptors look good!

The new field descriptors for version information are correctly defined following protobuf conventions.


6438-6440: Field initialization is properly implemented!

The new version fields are correctly initialized using the standard protobuf reflection pattern.


11153-11163: Message definition and getters are well-structured!

The new version fields are properly defined with:

  • Correct protobuf field numbers and tags
  • Proper JSON field name mapping
  • Standard Go getter methods

Also applies to: 11242-11261


11731-11743: Verify version compatibility annotations

All new fields are annotated with cosmos-sdk 0.52. Please ensure this version requirement is intentional and matches the planned release version.

✅ Verification successful

Version annotations are consistent with codebase changes

The version annotations using cosmos-sdk 0.52 are consistent with other new fields added in the same version. The proto definitions show that these version annotations are used to track field additions across different SDK versions, with other fields also using the same version pattern (e.g., comet_server_version, runtime_version, and stf_version all marked with cosmos-sdk 0.52).

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check for other usages of cosmos-sdk version annotations
rg --type=go "cosmos-sdk.*0\.52" -A 2 -B 2

Length of output: 44


Script:

#!/bin/bash
# Let's try a broader search for version annotations in proto/generated files
rg --type=go "cosmos-sdk" -A 2 -B 2

# Also search for version-related patterns in proto files
fd -e proto -x rg -l "cosmos-sdk"

Length of output: 70252


Script:

#!/bin/bash
# Let's search for version annotations in proto files specifically
fd -e proto -x cat {} \; | rg -A 2 -B 2 "cosmos-sdk.*0\.52|cosmos-sdk.*version"

# Also check for any version-related files or configuration
fd -g "*version*" -t f

Length of output: 1062

Comment on lines 89 to 94
Name: testName,
AppName: testAppName,
Version: testVersion,
GitCommit: testCommit,
BuildTags: testBuildTags,
CosmosSdkVersion: "unable to read deps",
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Enhance test coverage for v2 version information

The current test case doesn't adequately cover the new version information features described in the PR objectives:

  1. The hardcoded "unable to read deps" doesn't validate the actual version information that should be displayed.
  2. Missing test cases for:
    • Runtime/v2 version
    • STF version
    • Server/v2/cometbft version
  3. No validation of behavior when COSMOS_BUILD_OPTIONS=v2 is set

Consider adding these test cases:

 func TestCLI(t *testing.T) {
+    tests := []struct {
+        name        string
+        buildOpts   string
+        wantVersion version.Info
+    }{
+        {
+            name:      "with v2 build options",
+            buildOpts: "v2",
+            wantVersion: version.Info{
+                Name:               testName,
+                AppName:            testAppName,
+                Version:            testVersion,
+                GitCommit:          testCommit,
+                BuildTags:          testBuildTags,
+                RuntimeVersion:     "v2.0.0",  // Expected v2 runtime version
+                StfVersion:         "v1.0.0",  // Expected STF version
+                CometServerVersion: "v0.1.0",  // Expected server version
+            },
+        },
+        {
+            name:      "without v2 build options",
+            buildOpts: "",
+            wantVersion: version.Info{
+                Name:             testName,
+                AppName:          testAppName,
+                Version:          testVersion,
+                GitCommit:        testCommit,
+                BuildTags:        testBuildTags,
+            },
+        },
+    }
+
+    for _, tt := range tests {
+        t.Run(tt.name, func(t *testing.T) {
+            origBuildOpts := os.Getenv("COSMOS_BUILD_OPTIONS")
+            t.Cleanup(func() {
+                os.Setenv("COSMOS_BUILD_OPTIONS", origBuildOpts)
+            })
+            os.Setenv("COSMOS_BUILD_OPTIONS", tt.buildOpts)
+
+            // Your existing test logic here, but compare against tt.wantVersion
+        })
+    }
 }

Committable suggestion skipped: line range outside the PR's diff.

Copy link
Member

@julienrbrt julienrbrt left a comment

Choose a reason for hiding this comment

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

lgtm!

@julienrbrt
Copy link
Member

Could you check what's up: https://github.com/cosmos/cosmos-sdk/actions/runs/12244932192/job/34157747036?pr=22807

@julienrbrt julienrbrt added this pull request to the merge queue Dec 10, 2024
Merged via the queue into cosmos:main with commit 6a7ecd8 Dec 10, 2024
72 of 74 checks passed
mergify bot pushed a commit that referenced this pull request Dec 10, 2024
(cherry picked from commit 6a7ecd8)

# Conflicts:
#	CHANGELOG.md
#	api/cosmos/base/tendermint/v1beta1/query.pulsar.go
julienrbrt added a commit that referenced this pull request Dec 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport/v0.52.x PR scheduled for inclusion in the v0.52's next stable release C:CLI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update version command to display server/v2 info
3 participants