ref: new version in adodb publish now should be different, not greater#752
ref: new version in adodb publish now should be different, not greater#752
Conversation
WalkthroughThe pull request introduces a change in the version validation logic for the ADODB (Andromeda Database) functionality. The modification ensures that during the publish process, the new version must be different from the current version, rather than simply being greater. This change is reflected in the Changes
Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (6)
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 (
|
SlayerAnsh
left a comment
SlayerAnsh
left a comment
There was a problem hiding this comment.
In store_code_id, new version is automatically assigned to Latest version if its not a prerelease version. It should now have a check for new version being greater than Latest version to update Latest Version
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
contracts/os/andromeda-adodb/src/state.rs (2)
31-32: 🛠️ Refactor suggestionAvoid using
unwrap_orwith default values in critical logicUsing
unwrap_or(ADOVersion::from_string(String::default()))may lead to unintended behavior ifcurr_typeisNone, as it defaults to an emptyADOVersion, which may not matchado_version. Since you've already checkedcurr_type.is_none(), you can avoid the unnecessary unwrapping for clarity and correctness.
71-72: 🛠️ Refactor suggestionAvoid using
unwrap_orwith default values in critical logicSimilar to the earlier instance in
store_code_id, usingunwrap_or(ADOVersion::from_string(String::default()))may lead to unintended behavior. It's better to handle theOptionwithout defaulting to an emptyADOVersionto ensure the logical checks are accurate.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
contracts/os/andromeda-adodb/Cargo.toml(1 hunks)contracts/os/andromeda-adodb/src/execute.rs(2 hunks)contracts/os/andromeda-adodb/src/state.rs(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- contracts/os/andromeda-adodb/Cargo.toml
- contracts/os/andromeda-adodb/src/execute.rs
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: Contract Tests (ubuntu-latest, unit-test)
- GitHub Check: Contract Tests (ubuntu-latest, integration-test)
- GitHub Check: Contract Tests (ubuntu-latest, unit-test)
- GitHub Check: Contract Tests (ubuntu-latest, integration-test)
- GitHub Check: Schemas
- GitHub Check: Contracts
- GitHub Check: Summary
| let current_version = | ||
| semver::Version::parse(version).unwrap_or(Version::new(0, 0, 0)); | ||
| if penultimate_version < current_version && current_version < latest_version |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Review version selection logic in remove_code_id to align with the new versioning requirement
The current logic for finding the penultimate version relies on comparing versions using < operators to identify the next highest version below the latest. Since the requirement now is that the version must be different, not necessarily greater or lesser, this logic may not correctly identify the appropriate version to set as the new latest when the current latest version is removed. Consider adjusting the logic to select any different version as the new latest.
| let should_update = current_ado_version | ||
| .map(|(ver, _)| { | ||
| let current_version = semver::Version::parse(&ver).unwrap_or(Version::new(0, 0, 0)); | ||
| version > current_version | ||
| }) | ||
| .unwrap_or(true); // If there's no current version, we should update |
There was a problem hiding this comment.
Update version comparison logic to ensure the new version is different, not necessarily greater
The current logic checks if the new version is greater than the current version (version > current_version), but according to the PR objectives, the new version must be different from the current version, regardless of whether it's greater or lesser. To align with this requirement, please update the comparison to check for inequality instead.
Apply this diff to correct the logic:
- version > current_version
+ version != current_version📝 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.
| let should_update = current_ado_version | |
| .map(|(ver, _)| { | |
| let current_version = semver::Version::parse(&ver).unwrap_or(Version::new(0, 0, 0)); | |
| version > current_version | |
| }) | |
| .unwrap_or(true); // If there's no current version, we should update | |
| let should_update = current_ado_version | |
| .map(|(ver, _)| { | |
| let current_version = semver::Version::parse(&ver).unwrap_or(Version::new(0, 0, 0)); | |
| version != current_version | |
| }) | |
| .unwrap_or(true); // If there's no current version, we should update |
crnbarr93
left a comment
crnbarr93
left a comment
There was a problem hiding this comment.
We should also add unit tests for this to ensure designed outcome.
If an ADO has version 1.0.0-b.1 then the following versions should be valid:
- 1.0.0-b.2
- 1.0.1
- 1.0.1-b.x
- 1.1.0
- 1.1.0-b.x
- 2.0.0
- 2.0.0-b.x
As they all bump at least one of the elements of the version. However only a strictly greater than should update the LATEST_VERSION.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
contracts/os/andromeda-adodb/src/tests.rs (1)
754-771: Consider adding documentation for test cases.While the test cases are well-structured, adding documentation to explain the purpose and expected behavior of each test case would improve maintainability.
+// Test cases for version updates: +// 1. Beta version scenarios: +// - Increment beta version number +// - Transition from beta to regular version +// - Maintain beta status with version changes +// 2. Regular version scenarios: +// - Patch, minor, and major increments +// - Transition to beta versions +// - Lower version handling #[rstest] #[case("1.0.0-b.1", "1.0.0-b.2", false, true)] // Beta version increment #[case("1.0.0-b.1", "1.0.1", true, true)] // Patch increment from beta
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
contracts/os/andromeda-adodb/Cargo.toml(2 hunks)contracts/os/andromeda-adodb/src/state.rs(4 hunks)contracts/os/andromeda-adodb/src/tests.rs(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- contracts/os/andromeda-adodb/Cargo.toml
- contracts/os/andromeda-adodb/src/state.rs
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Summary
🔇 Additional comments (3)
contracts/os/andromeda-adodb/src/tests.rs (3)
12-12: LGTM! Good use of parameterized testing.The addition of the
rstestcrate is a good choice for organizing test cases.
722-726: LGTM! Improved readability.The array initialization is now more readable with each version on a separate line.
754-771: LGTM! Comprehensive test coverage for version updates.The test cases thoroughly cover various version update scenarios including beta versions, regular versions, and their transitions.
crnbarr93
left a comment
crnbarr93
left a comment
There was a problem hiding this comment.
Small version change then LGTM!
| .for_each(|v| { | ||
| if let Some((_, version)) = v.split_once('@') { | ||
| let current_version = semver::Version::parse(version).unwrap(); | ||
| let current_version = |
There was a problem hiding this comment.
Remove doesn't check if the new latest version is prerelease version or not. Pre release can never be latest according to our store code logic.
3 participants
Motivation
The version provided while publishing to the ADODB should now be different than the current version, not necessarily greater.
Implementation
Testing
No tests were impacted
Version Changes
andromeda-adodb:1.1.2->1.1.3Notes
Removed some outdated TODO comments, reworded the contract error accordingly for the above change.
Future work
None
Summary by CodeRabbit
Summary by CodeRabbit
Changed
1.1.3-b.1.Chores
rstestfor parameterized testing and introduced new test scenarios for version updates.andromeda-adodb.