-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
chore(tools/cosmovisor): add multiarch bin references #21911
chore(tools/cosmovisor): add multiarch bin references #21911
Conversation
required by cosmos#21891 allows cosmovisor tests to natively run on following os/arch pairs linux/amd64, linux/arm64 darwin/amd64, darwin/arm64 Signed-off-by: Artur Troian <[email protected]>
📝 Walkthrough📝 WalkthroughWalkthroughThe changes involve modifications to the upgrade information generation for a software upgrade process within the Cosmovisor tool. The updates include transitioning from a single-line JSON format to a multi-line heredoc format for improved readability. Additionally, the upgrade information now supports multiple binary links for various operating systems, specifically adding support for Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Cosmovisor
participant Repository
User->>Cosmovisor: Request upgrade information
Cosmovisor->>Repository: Fetch upgrade data
Repository-->>Cosmovisor: Return JSON with binary links
Cosmovisor-->>User: Provide upgrade information
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? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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)
Other keywords and placeholders
Documentation and Community
|
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.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
tools/cosmovisor/testdata/download/cosmovisor/genesis/bin/autod (1)
9-15
: Approved: Multiarch support added successfullyThe changes successfully implement the multiarch binary references as per the PR objectives. The use of a heredoc for the JSON structure improves readability. The upgrade info now includes binary links for linux/amd64, linux/arm64, darwin/amd64, and darwin/arm64, which aligns with the goal of supporting multiple OS and architecture combinations.
Consider adding a comment explaining the purpose of the checksum in the binary URLs. This would enhance code maintainability. For example:
# The checksum in the URL is used to verify the integrity of the downloaded binary
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (3)
- tools/cosmovisor/testdata/download/cosmovisor/genesis/bin/autod (1 hunks)
- tools/cosmovisor/testdata/repo/chain2-zip_bin/autod (1 hunks)
- tools/cosmovisor/testdata/repo/ref_to_chain3-zip_dir.json (1 hunks)
🔇 Additional comments not posted (2)
tools/cosmovisor/testdata/repo/ref_to_chain3-zip_dir.json (1)
3-6
: Verify architecture-specific binaries and update file namingThe changes successfully add support for multiple architectures (linux/arm64, darwin/amd64, darwin/arm64) alongside the existing linux/amd64. However, there are some concerns:
All architectures are pointing to the same binary file (
autod.zip
). This might not be correct as typically, different architectures require different binary builds.The filename
autod.zip
doesn't indicate any architecture specificity, which is unusual for multi-architecture support.Please confirm that:
- The same binary is indeed intended to work across all these architectures.
- If not, update the URLs to point to architecture-specific binaries.
- Consider updating the filename to include architecture information (e.g.,
autod-linux-amd64.zip
,autod-darwin-arm64.zip
, etc.) for clarity.To verify the contents of the zip file, you can run:
This will help ensure that the zip file contains the correct binaries for all supported architectures.
tools/cosmovisor/testdata/repo/chain2-zip_bin/autod (1)
9-15
: Improved JSON structure with multi-architecture supportThe changes enhance the upgrade information generation process:
- The switch from a single-line echo to a multi-line heredoc improves readability and maintainability.
- The JSON structure now includes binary references for multiple architectures (linux/amd64, linux/arm64, darwin/amd64, darwin/arm64), which aligns with the PR objective of supporting multiarch environments.
- The URL and checksum are consistent across all architectures, which is correct if the same binary is compatible with all listed architectures.
To ensure the consistency of the checksum across different parts of the codebase, run the following script:
✅ Verification successful
Checksum Consistently Used as Intended
The checksum
a95075f4dd83bc9f0f556ef73e64ce000f9bf3a6beeb9d4ae32f594b1417ef7a
is consistently used intools/cosmovisor/testdata/repo/chain2-zip_bin/autod
without any conflicting instances elsewhere in the codebase.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the consistency of the checksum used in the upgrade information. # Test: Search for the checksum value in the codebase. Expect: Consistent usage. rg "a95075f4dd83bc9f0f556ef73e64ce000f9bf3a6beeb9d4ae32f594b1417ef7a"Length of output: 975
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.
utACK
fixes incorrect checksums introduced in cosmos#21911 Signed-off-by: Artur Troian <[email protected]>
fixes cosmos#21911 cosmos#21933 Signed-off-by: Artur Troian <[email protected]>
required by #21891
allows cosmovisor tests to natively run on following os/arch pairs linux/amd64, linux/arm64
darwin/amd64, darwin/arm64
Description
Closes: #XXXX
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...
!
in the type prefix if API or client breaking changeCHANGELOG.md
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...
Summary by CodeRabbit
New Features
Bug Fixes