Skip to content
This repository was archived by the owner on Nov 15, 2023. It is now read-only.

Conversation

@ordian
Copy link

@ordian ordian commented Aug 4, 2022

Alternative to #5858.

See #5858 (comment) for justification.

The original purpose of the check was to ensure we bump the spec_version on releases properly based on what's changed in master. Our current release process bumps it always, making this check obsolete.

One downside I can think of is a potential of getting "state root mismatch" when running e.g. burn-in from master.
To avoid that, we could:

  • set the spec_version in master to some different value (e.g. 999999) than in release branches
  • make spec_version bump in master a part of the release process: Update release issue template #4268

@ordian ordian added A2-insubstantial Pull request requires no code review (e.g., a sub-repository hash update). B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. labels Aug 4, 2022
@ordian ordian requested review from a team and bkchr August 4, 2022 13:07
@ordian ordian requested review from a team and chevdor as code owners August 4, 2022 13:07
@paritytech-ci paritytech-ci requested a review from a team August 4, 2022 14:16
@chevdor
Copy link
Contributor

chevdor commented Aug 5, 2022

We should keep this test and only remove it from non release branches and especially master.

It is good to have the check on the release branches since we have it. I would prefer how it is done in #5858
The check should never fail (besides if we create the release branch and the versions were not bumped yet but we are working on scripts that will prevent this case) and if it does, it means we messed up and really want to know about it.

Nowadays, the version in master should be bumped to the same version that we just released.
This is usually done in || and we make a PR for the release and the same for master.

I don't think it is a good idea to use a bigger runtime spec_version such as 9999 for master.
Say we just released 9270, is there any issue with master now being 9270 until the next release ?

@ordian
Copy link
Author

ordian commented Aug 5, 2022

Say we just released 9270, is there any issue with master now being 9270 until the next release ?

An issue mentioned in the description (only relevant for the native runtime):

a potential of getting "state root mismatch" when running e.g. burn-in from master

So once we release 9270, the master should be bumped to 9280 (or, alternatively, master should be permanently on 99..99).

It is good to have the check on the release branches

We could keep it for release branches, but then we won't see this check on PRs on github to release branches. Would it still be useful this way? I guess this is more of a question for release engineers.

If we keep this check on PRs, then the release engineering team should bump the spec_version on master. Otherwise, as soon as the release is done, all PRs turn red, although they haven't changed anything. This is very annoying.

@bkchr
Copy link
Member

bkchr commented Aug 5, 2022

I think we should just remove it. If someone wants it for the release branch or whatever, it can come back. However, each release we have this test failing for multiple days and it is just annoying!

Anyone knows why the stable job didn't return any status? Is it maybe depending somehow on this check-runtime check?

@ordian
Copy link
Author

ordian commented Aug 5, 2022

Anyone knows why the stable job didn't return any status? Is it maybe depending somehow on this check-runtime check?

My bad, should be fixed now.

#5858 now also disables it for PRs, so it comes down to whether release engineers find this check being run on release branch (but not PRs to it) useful or not

@alvicsam
Copy link
Contributor

I merged #5858 since @chevdor confirmed that release team still needs that job. Probably we can close this PR

@ordian ordian closed this Aug 11, 2022
@ordian ordian deleted the ao-ci-remove-check-runtime branch August 11, 2022 14:29
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

A2-insubstantial Pull request requires no code review (e.g., a sub-repository hash update). B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants