-
Notifications
You must be signed in to change notification settings - Fork 39
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(dashmate): set tenderdash version to 1 #2385
Conversation
WalkthroughThe changes involve updates to the Docker image version for the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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
CodeRabbit Configuration File (
|
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: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/dashmate/configs/defaults/getBaseConfigFactory.js
(1 hunks)packages/dashmate/configs/getConfigFileMigrationsFactory.js
(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/dashmate/configs/defaults/getBaseConfigFactory.js
'1.7.0': (configFile) => { | ||
Object.entries(configFile.configs) | ||
.forEach(([, options]) => { | ||
options.platform.drive.tenderdash.docker.image = 'dashpay/tenderdash:1'; | ||
}); | ||
return configFile; | ||
}, |
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.
💡 Codebase verification
Migration introduces version regression from 1.3 to 1
The version history shows a concerning regression in tenderdash versions:
- Version 1.1.1: Sets to 'dashpay/tenderdash:1.2.0'
- Version 1.3.0: Updates to 'dashpay/tenderdash:1.3'
- Version 1.4.0-dev.1: Maintains 'dashpay/tenderdash:1.3'
- Version 1.7.0 (current): Downgrades to 'dashpay/tenderdash:1'
This migration appears to be downgrading the tenderdash version from 1.3 to 1, which is likely unintended and could cause compatibility issues. The version should either maintain 1.3 or upgrade to a newer version, but not downgrade.
🔗 Analysis chain
LGTM! Migration function correctly updates tenderdash version.
The migration function follows the established pattern and properly updates the tenderdash Docker image version to '1' across all configurations, which aligns with the PR objectives.
Let's verify that this is the only place where the tenderdash version needs to be updated:
Tenderdash version update requires additional changes
The migration function needs to be reviewed as there are inconsistencies in tenderdash versioning across the codebase:
packages/dashmate/configs/defaults/getBaseConfigFactory.js
sets default image to 'dashpay/tenderdash:1'- Multiple migrations in
getConfigFileMigrationsFactory.js
show version changes:- From 0.13.2 (in test fixtures) to 1.2.0
- Then to 1.3
- And now attempting to change to 1
This suggests a potential regression as we're moving from version 1.3 to 1, which might not be intended. Let's verify the version history:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for other occurrences of tenderdash version specifications
# Test: Search for tenderdash Docker image version declarations
rg "dashpay/tenderdash:[0-9]" --type js
Length of output: 1334
Script:
#!/bin/bash
# Get the chronological order of migrations to understand version progression
ast-grep --pattern 'options.platform.drive.tenderdash.docker.image = $_' packages/dashmate/configs/getConfigFileMigrationsFactory.js -A 5 -B 5
Length of output: 25532
@@ -310,7 +310,7 @@ export default function getBaseConfigFactory() { | |||
tenderdash: { | |||
mode: 'full', | |||
docker: { | |||
image: 'dashpay/tenderdash:1.3', | |||
image: 'dashpay/tenderdash:1', |
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 still believe this should be 1.4. Using 1 can cause use of - for example - 1.0.0 (if it's already pulled) which is not compatible.
We had issues with drive where people didn't do dashmate update (or it failed?) and were reporting issues. Using more precise tag should fix this issue, and enforce update even if the user doesn't call dashmate update
directly.
And it will protect against unintended minor version updates.
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.
We use the same approach for Core (only major version is specified).
Will you change your opinion if dashmate will automatically pull images after updating the package version?
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.
This will definitely help, but will still not be perfect. The issue is that docker does not support semver :)
Accepting for now, but needs more discussion.
Issue being fixed or feature implemented
Tenderdash is updated to version 1
What was done?
How Has This Been Tested?
None
Breaking Changes
None
Checklist:
For repository code-owners and collaborators only
Summary by CodeRabbit
tenderdash
component to version1
.drive
platform.