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

feat: throw/warn on mismatch of client version and supported version #1287

Merged
merged 23 commits into from
Oct 3, 2023
Merged

feat: throw/warn on mismatch of client version and supported version #1287

merged 23 commits into from
Oct 3, 2023

Conversation

nedsalk
Copy link
Contributor

@nedsalk nedsalk commented Sep 22, 2023

Closes #1283 .

Throws when major and minor versions mismatch, and warns when patch versions mismatch.
After we go live, we can change the implementation to warn on minor version mismatch. Currently, our minor versions contain breaking changes so we should throw.

@nedsalk nedsalk self-assigned this Sep 22, 2023
@nedsalk nedsalk enabled auto-merge (squash) September 22, 2023 08:02
@github-actions
Copy link
Contributor

github-actions bot commented Sep 22, 2023 •
edited

Coverage report

St.❔
 Category Percentage Covered / Total
🟢 Statements
85.02% (+0.03% 🔼)
 5229/6150
🟡 Branches
66.61% (+0.12% 🔼)
 766/1150
🟡 Functions
75% (-0.04% 🔻)
 855/1140
🟢 Lines
84.92% (+0.03% 🔼)
 5011/5901
Show new covered files 🐣
St.❔
 File Statements Branches Functions Lines
🟢
... / checkFuelCoreVersionCompatibility.ts
 100% 100% 100% 100%
Show files with reduced coverage 🔻
St.❔
 File Statements Branches Functions Lines
🟡
... / provider.ts
61.81% (+0.68% 🔼)
50.67% (+2.78% 🔼)
58.21% (-0.88% 🔻)
62.86% (+0.67% 🔼)

Test suite run success

1252 tests passing in 216 suites.

Report generated by 🧪jest coverage report action from e1b789a

arboleya
arboleya previously requested changes Sep 23, 2023
View reviewed changes
Copy link
Member

@arboleya arboleya left a comment

Choose a reason for hiding this comment

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

This is unrelated to your changes, but it'd be nice if we could fix the Forbidden non-null assertion warnings.

screenshot

warnings

packages/providers/src/provider.ts Outdated Show resolved Hide resolved
@nedsalk
Copy link
Contributor Author

nedsalk commented Sep 26, 2023

@arboleya

This is unrelated to your changes, but it'd be nice if we could fix the Forbidden non-null assertion warnings.

These warnings should be sorted out repo-wide by changing linting rules to treat them as errors. IMO that should be a separate PR.

@arboleya
Copy link
Member

These warnings should be sorted out repo-wide [..]

@nedsalk Nevermind, no need for "repo-wide" orphan issues.

Thank you for fixing them on your other PR.

nedsalk and others added 3 commits September 27, 2023 08:15
@nedsalk @arboleya
Update packages/versions/src/lib/checkFuelCoreVersionCompatibility.ts
c3c1f84 
Co-authored-by: Anderson Arboleya <anderson@arboleya.me>
@nedsalk @arboleya
Update packages/versions/src/lib/checkFuelCoreVersionCompatibility.te…
25ecd96 
…st.ts

Co-authored-by: Anderson Arboleya <anderson@arboleya.me>
@nedsalk
fix: changes to property name
64113b9
arboleya
arboleya requested changes Sep 27, 2023
View reviewed changes
Copy link
Member

@arboleya arboleya left a comment

Choose a reason for hiding this comment

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

Thanks again. Unfortunately, I made a mistake in my last suggestion.

Also, you should re-request reviews instead of dismissing them as stale.

There are two ways of doing so.

screenshots

re1

re2

packages/versions/src/lib/checkFuelCoreVersionCompatibility.test.ts Outdated Show resolved Hide resolved
@nedsalk @arboleya
Update packages/versions/src/lib/checkFuelCoreVersionCompatibility.te…
1d58531 
…st.ts

Co-authored-by: Anderson Arboleya <anderson@arboleya.me>
arboleya
arboleya previously approved these changes Sep 27, 2023
View reviewed changes
Copy link
Member

@arboleya arboleya left a comment

Choose a reason for hiding this comment

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

Thank you. ⚡

camsjams
camsjams previously approved these changes Sep 27, 2023
View reviewed changes
Copy link
Contributor

@camsjams camsjams left a comment

Choose a reason for hiding this comment

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

:shipit:

@Dhaiwat10
Copy link
Member

@nedsalk looks like this PR is ready to go but the test check is failing

@nedsalk
Copy link
Contributor Author

nedsalk commented Sep 30, 2023

@Dhaiwat10 yes, the mocks failed after some changes got introduced. I have a fix in mind but have to focus on more urgent stuff currently.

@nedsalk nedsalk dismissed stale reviews from camsjams and arboleya via e1b789a October 3, 2023 12:05
@nedsalk nedsalk merged commit 1d5d3e1 into master Oct 3, 2023
8 checks passed
@nedsalk nedsalk deleted the ns/feat/check-node-version-in-provider branch October 3, 2023 12:29
@github-actions github-actions bot mentioned this pull request Oct 3, 2023
Merged
@camsjams camsjams mentioned this pull request Oct 10, 2023
Closed
@Br1ght0ne Br1ght0ne mentioned this pull request Oct 12, 2023
Merged
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Torres-ssf Torres-ssf Torres-ssf approved these changes

@Dhaiwat10 Dhaiwat10 Dhaiwat10 approved these changes

@arboleya arboleya arboleya left review comments

@camsjams camsjams camsjams left review comments

@digorithm digorithm Awaiting requested review from digorithm digorithm is a code owner

@danielbate danielbate Awaiting requested review from danielbate danielbate is a code owner

Assignees

@nedsalk nedsalk

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Check the provider's version and throw an error (warning?) if the version is old and unsupported
5 participants
@nedsalk @arboleya @Dhaiwat10 @camsjams @Torres-ssf