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

Accept T2 by B1 mention check #10

Merged
merged 3 commits into from
Jan 27, 2023
Merged

Accept T2 by B1 mention check #10

merged 3 commits into from
Jan 27, 2023

Conversation

ggwpez
Copy link
Member

@ggwpez ggwpez commented Jan 26, 2023

Changes to a runtime API need a B1 label, but B1 rejects T2. Therefore:

  • Accept the T2 label when B1 is specified

MR paritytech/polkadot#6536 and paritytech/cumulus#2073 suffer from this.
TODO:

  • Add tests

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
@ggwpez ggwpez closed this Jan 26, 2023
@ggwpez ggwpez reopened this Jan 26, 2023
Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
@chevdor
Copy link
Contributor

chevdor commented Jan 26, 2023

After some checking with Oliver, I think we can summarize this way:

The issue is that T0 and T1 are orthogonal: We can (according to our rules) have only one of them and we must have at least one of them.

Adding T2 is not an issue. But removing T1 while not having a T0 instead is not legal with the current rules.

@the-right-joyce it may be easier to move T0 and T1 away from the Ts and keep them with a letter that is exclusive.
That would also free 2 spots in the non exclusive Ts.

For now, it sounds like @ggwpez can live with adding back T1 on the mentioned PRs.

@chevdor
Copy link
Contributor

chevdor commented Jan 26, 2023

There are 2 rules at play here and I missed that in my inital review:
https://github.com/paritytech/labels/blob/main/ruled_labels/specs_cumulus.yaml#L219-L231

Those say:

  • if we have B1, we must have 1 and only 1 of the set [T0, T1]
  • if we have B1, we must have 1..n of the set [T*]

We need to make both those rules happy.

@ggwpez
Copy link
Member Author

ggwpez commented Jan 27, 2023

Going to merge this now since I think its a valid use-case. However, in the MR that i mentioned it probably makes more sens to use both labels, since it does slightly alter the runtime blob.

@ggwpez ggwpez merged commit 7b92cbf into main Jan 27, 2023
@ggwpez ggwpez deleted the oty-t2 branch January 27, 2023 16:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants