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.

Sign up for GitHub

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

Jump to bottom

fix: make single guardian devimint cli test backwards-compatible #4104

Merged
merged 2 commits into from
Jan 25, 2024
Merged

fix: make single guardian devimint cli test backwards-compatible #4104

merged 2 commits into from
Jan 25, 2024

Conversation

bradleystachurski
Copy link
Member

Fixes #4017

I verified locally against the full matrix of backwards-compatibility tests that devimint_cli_test_single succeeds (see: #4010).

Backwards-compatibility tests summary:
fed_version  client_version  gateway_version  exit_code
v0.2.1       v0.2.1          current          0
v0.2.1       current         v0.2.1           0
v0.2.1       current         current          0
current      v0.2.1          v0.2.1           0
current      v0.2.1          current          0
current      current         v0.2.1           0

@bradleystachurski bradleystachurski requested a review from a team as a code owner January 23, 2024 06:49
@codecov Codecov
Copy link

codecov bot commented Jan 23, 2024 •
edited

Codecov Report

Attention: 21 lines in your changes are missing coverage. Please review.

Comparison is base (320dbc7) 58.35% compared to head (029921d) 58.30%.
Report is 31 commits behind head on master.

Files Patch % Lines
devimint/src/main.rs 0.00% 12 Missing ⚠️
devimint/src/util.rs 68.96% 9 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##           master    #4104      +/-   ##
==========================================
- Coverage   58.35%   58.30%   -0.06%     
==========================================
  Files         194      192       -2     
  Lines       42581    42740     +159     
==========================================
+ Hits        24847    24918      +71     
- Misses      17734    17822      +88

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@bradleystachurski bradleystachurski force-pushed the fix-single-guardian branch 4 times, most recently from ee13d4a to df628a0 Compare January 23, 2024 15:43
elsirion
elsirion previously approved these changes Jan 24, 2024
View reviewed changes
Copy link
Contributor

@elsirion elsirion left a comment

Choose a reason for hiding this comment

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

🎉

dpc
dpc reviewed Jan 24, 2024
View reviewed changes
devimint/src/main.rs Outdated
let fedimintd_version_res = cmd!(devimint::util::FedimintdCmd, "--version")
.out_string()
.await;
if devimint::util::is_min_version(0, 3, 0, fedimintd_version_res) {
Copy link
Contributor

Choose a reason for hiding this comment

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

is_min_version confuses me. Would expect is_version_at_least or is_version_gt kind of things.

Also 0, 3, 0 ... looks weird.

dpc
dpc reviewed Jan 24, 2024
View reviewed changes
devimint/src/util.rs Outdated
/// min version provided. If there's an error parsing the version, returns
/// false.
pub fn is_min_version(
min_major: u32,
Copy link
Contributor

Choose a reason for hiding this comment

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

If we have CrateVersion why not use it here?

dpc
dpc reviewed Jan 24, 2024
View reviewed changes
devimint/src/util.rs Outdated
min_major: u32,
min_minor: u32,
min_patch: u32,
version_res: Result<String, anyhow::Error>,
Copy link
Contributor

Choose a reason for hiding this comment

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

Taking Result<String, anyhow::Error> here is odd and confusing.

If for some reason older versions didn't have --version or something like that, then the version parsing function should default to some magic number (0.2.0? 0.1.0?) to unify all the cases.

Copy link
Contributor

Choose a reason for hiding this comment

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

Without these issues, we can use some more natural version comparison (I'm not sure if cmp::Ordering and operators like <= could be used for semver, need more coffee).

BTW. https://docs.rs/semver/latest/semver/ exists and is not a heavy crate, maybe we should just use it?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good call 👍

If we're comfortable introducing the semver crate, it simplifies this PR.

029921d

@bradleystachurski bradleystachurski force-pushed the fix-single-guardian branch 2 times, most recently from 5bc44aa to 1e367d2 Compare January 25, 2024 02:53
dpc
dpc approved these changes Jan 25, 2024
View reviewed changes
Copy link
Contributor

@dpc dpc left a comment

Choose a reason for hiding this comment

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

:shipit:

@elsirion elsirion added this pull request to the merge queue Jan 25, 2024
Merged via the queue into fedimint:master with commit a099fff Jan 25, 2024
20 checks passed
@bradleystachurski bradleystachurski deleted the fix-single-guardian branch January 25, 2024 13:49
@bradleystachurski bradleystachurski mentioned this pull request Jan 25, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dpc dpc dpc approved these changes

@elsirion elsirion elsirion approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

devimint_cli_test_single fails backwards-compatibility test
3 participants
@bradleystachurski @dpc @elsirion