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

Companion PR for #6846 #1568

Merged
4 commits merged into from
Aug 15, 2020
Merged

Companion PR for #6846 #1568

4 commits merged into from
Aug 15, 2020

Conversation

seunlanlege
Copy link
Contributor

Companion PR for paritytech/substrate#6846

@github-actions github-actions bot added the A0-please_review Pull request needs code review. label Aug 11, 2020
@seunlanlege seunlanlege added the A2-insubstantial Pull request requires no code review (e.g., a sub-repository hash update). label Aug 11, 2020
@seunlanlege
Copy link
Contributor Author

Honestly not sure what I'm doing wrong, but isn't CI supposed to automagically figure out that this a companion pr and adjust the branch accordingly?

@expenses
Copy link
Contributor

@seunlanlege #1549 not any more.

@expenses
Copy link
Contributor

I think you just ignore the CI failures.

@cecton
Copy link
Contributor

cecton commented Aug 12, 2020

Yes! Thank you. Just ignore the failures and re-run the build when substrate has been merge (or just use bot merge and it should merge anyway) @seunlanlege

copy-pasting what I said the other day:

  1. you push your companion PR
  2. you DON'T update the substrate branch of your companion PR
  3. you let the CI be red, this is expected
  4. you wait for substrate to be green and merged
  5. then only you retrigger the builds in polkadot
  6. merge

@ghost
Copy link

ghost commented Aug 15, 2020

Waiting for commit status.

@cecton
Copy link
Contributor

cecton commented Aug 15, 2020

@sjeohp This should not happen IMO. This branch has already been tested in substrate with check-polkadot-companion-build.

Now this build is running in polkadot but substrate has been merged. Everybody who is right now trying to make a branch on substrate with a companion branch are stuck because substrate and polkadot are out-of-sync for some time (the time of this build).

Would it be possible to bypass the checks and make the merge happens immediately (in general, I'm not talking about this PR as it is the weekend anyway)?

cc @TriplEight @kianenigma

@cecton
Copy link
Contributor

cecton commented Aug 15, 2020

(I mean it is fine that the Cargo.lock is updated before merge but it shouldn't wait for the status to succeed)

@ghost
Copy link

ghost commented Aug 15, 2020

Checks failed; merge aborted.

@cecton
Copy link
Contributor

cecton commented Aug 15, 2020

I waited this build to show the problem:

image

The build failed because the labels are not set properly. But we don't care because it really is a change in substrate where there are labels already.

But now we have a permanent state where nobody can merge their PR on substrate until this issue is manually fixed by someone. This should not happen: if substrate succeeded, it should be merged right away, without additional check, without additional build time.

cc @sjeohp @TriplEight @kianenigma

@cecton cecton added 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 15, 2020
@cecton
Copy link
Contributor

cecton commented Aug 15, 2020

bot merge

@ghost
Copy link

ghost commented Aug 15, 2020

Waiting for commit status.

@ghost ghost merged commit d81c98b into master Aug 15, 2020
@ghost ghost deleted the seun-substrate-test-runner branch August 15, 2020 09:31
@sjeohp-zz
Copy link

Would it be possible to bypass the checks and make the merge happens immediately (in general, I'm not talking about this PR as it is the weekend anyway)?

@cecton It would mean granting processbot admin privileges I think, where currently it is only maintainer.

ordian added a commit that referenced this pull request Aug 17, 2020
* master:
  Companion PR for #6862 (#1564)
  implement collation generation subsystem (#1557)
  Add spawn_blocking to SubsystemContext (#1570)
  Companion PR for #6846 (#1568)
  overseer: add a test to ensure all subsystem receive msgs (#1590)
  Implementer's Guide: Flesh out more details for upward messages (#1556)
ordian added a commit that referenced this pull request Aug 17, 2020
…n-race-condition

* master:
  Companion PR for #6862 (#1564)
  implement collation generation subsystem (#1557)
  Add spawn_blocking to SubsystemContext (#1570)
  Companion PR for #6846 (#1568)
  overseer: add a test to ensure all subsystem receive msgs (#1590)
  Implementer's Guide: Flesh out more details for upward messages (#1556)
  Make parachain validation wasm executor functional (#1574)
  Use async test helper to simplify node testing (#1578)
  guide: validation data refactoring (#1576)
  Remove v0 parachains runtime (#1501)
  [CI] Add github token to generate-release-text (#1581)
  Allow using any polkadot client instead of enum Client (#1575)
  service/src/lib: Update authority discovery construction (#1563)
ordian added a commit that referenced this pull request Aug 17, 2020
* master:
  overseer: fix build (#1596)
  Companion PR for #6862 (#1564)
  implement collation generation subsystem (#1557)
  Add spawn_blocking to SubsystemContext (#1570)
  Companion PR for #6846 (#1568)
  overseer: add a test to ensure all subsystem receive msgs (#1590)
  Implementer's Guide: Flesh out more details for upward messages (#1556)
  Make parachain validation wasm executor functional (#1574)
  Use async test helper to simplify node testing (#1578)
  guide: validation data refactoring (#1576)
  Remove v0 parachains runtime (#1501)
  [CI] Add github token to generate-release-text (#1581)
  Allow using any polkadot client instead of enum Client (#1575)
  service/src/lib: Update authority discovery construction (#1563)
  Update .editorconfig to what we have in practice (#1545)
  Companion PR for substrate #6672 (#1560)
  pre-redenomination tockenSymbol change (#1561)
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review. 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.

4 participants