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.

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

Fix migration & it-tests for release-v0.5.4 #1356

Closed
wants to merge 5 commits into from

Conversation

Chralt98
Copy link
Member

@Chralt98 Chralt98 commented Aug 13, 2024

What does it do?

  • Fix migration for battery station to remove markets, which fail to decode
  • Replace main-net RPC endpoint for try-runtime
  • Correct the pallet storage version for market-commons to match the newest version
  • added bool flag to VERBOSE_LOG to satisfy moonwall

What important points should reviewers know?

Is there something left for follow-up PRs?

What alternative implementations were considered?

Are there relevant PRs or issues?

References

@Chralt98 Chralt98 self-assigned this Aug 13, 2024
@Chralt98 Chralt98 changed the base branch from main to release-v0.5.4 August 13, 2024 13:30
@Chralt98 Chralt98 added the s:in-progress The pull requests is currently being worked on label Aug 13, 2024
@Chralt98 Chralt98 changed the title Fix migration for release-v0.5.4 Fix migration & it-tests for release-v0.5.4 Aug 14, 2024
@Chralt98 Chralt98 marked this pull request as ready for review August 14, 2024 09:47
@Chralt98 Chralt98 added s:review-needed The pull request requires reviews and removed s:in-progress The pull requests is currently being worked on labels Aug 14, 2024
@codecov-commenter
Copy link

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 57.14286% with 3 lines in your changes missing coverage. Please review.

Please upload report for BASE (release-v0.5.4@0b858a2). Learn more about missing BASE report.

Files Patch % Lines
zrml/market-commons/src/migrations.rs 57.14% 3 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@                Coverage Diff                @@
##             release-v0.5.4    #1356   +/-   ##
=================================================
  Coverage                  ?   93.24%           
=================================================
  Files                     ?      133           
  Lines                     ?    29557           
  Branches                  ?        0           
=================================================
  Hits                      ?    27560           
  Misses                    ?     1997           
  Partials                  ?        0           
Flag Coverage Δ
tests 93.24% <57.14%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

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

Copy link
Member

@maltekliemann maltekliemann 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 currently missing tests. Can we add tests or is this scenario difficult to reproduce?

Also, I think if you're going to hardcode the market IDs, which I understand is a practical approach, you should make this a separate migration which we then only apply to Battery Station using the with-battery-station-runtime feature.

Please also change the target branch to main and then merge main into the release branch after merging this feature branch.

@Chralt98 Chralt98 added s:in-progress The pull requests is currently being worked on and removed s:review-needed The pull request requires reviews labels Aug 14, 2024
@Chralt98 Chralt98 changed the base branch from release-v0.5.4 to main August 14, 2024 10:26
@Chralt98 Chralt98 requested a review from sea212 as a code owner August 14, 2024 10:26
@Chralt98 Chralt98 added s:abandoned This pull request is abandoned and removed s:in-progress The pull requests is currently being worked on labels Aug 14, 2024
@Chralt98
Copy link
Member Author

Abandoned in favor of this PR #1357, because of the commit history that includes the version bumps, which should not be part of the merge to main.

@Chralt98 Chralt98 closed this Aug 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
s:abandoned This pull request is abandoned
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants