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

Incorrect Error: It's not possible to meet the minimal approval count of 1 #347

Closed
koute opened this issue Nov 9, 2021 · 3 comments
Closed

Incorrect Error: It's not possible to meet the minimal approval count of 1 #347

koute opened this issue Nov 9, 2021 · 3 comments

Comments

@koute
Copy link

koute commented Nov 9, 2021

I tried to merge this PR:

paritytech/substrate#10149

while I had 1 approval (from bkchr) in my companion:

paritytech/polkadot#4210

and yet the bot gave me this error:

Error: It's not possible to meet the minimal approval count of 1 in paritytech/polkadot#4210

Only after @ordian added his own approval to paritytech/polkadot#4210 did the "bot merge" work in paritytech/substrate#10149.

After this the Polkadot companion merge failed anyway which may or may not be related. (Although I'd argue it shouldn't fail; isn't the whole point of the bot to make the merge across two repositories atomic?)
@ordian
Copy link
Member

ordian commented Nov 9, 2021

I suspect the error message was incorrect and it was expecting 2 approvals: #319.

Also see #327.

@joao-paulo-parity joao-paulo-parity transferred this issue from paritytech/pipeline-scripts Dec 14, 2021
@joao-paulo-parity
Copy link
Contributor

joao-paulo-parity commented Dec 15, 2021 •
edited
Loading

After this the Polkadot companion merge failed anyway which may or may not be related. (Although I'd argue it shouldn't fail; isn't the whole point of the bot to make the merge across two repositories atomic?)

The current design inherently is flawed in this regard. For the merge to be "atomic" we'd need:

  1. Full CI integration (https://github.com/paritytech/ci_cd/issues/234#issue-1026474765) so that all PRs are green and therefore the API merge call can be fired simultaneously on all of them at once. Currently we first merge one PR while their companions are red under the pretense of "after updating the reference, the companion will become green" but obviously there's no guarantee that this is actually the case in practice - for instance, we've seen the companion fail due to missing labels as reported in https://github.com/paritytech/ci_cd/issues/159#issuecomment-899549998.
  2. A queue which prioritizes companions, which was brought up before

It is necessary to redesign the companion build system - #327 and many other stray comments in this issue tracker hint at that - or possibly move it out of the CI pipeline so that we'll have less friction for developers and external contributors alike. I have been talking with @TriplEight about those ideas precisely. This year I did not have time to work on this - had too many projects + I failed to raise exposure for this problem - but it's something that should be dealt with.

joao-paulo-parity added a commit to paritytech/substrate that referenced this issue Dec 23, 2021
@joao-paulo-parity
Make check-dependent-* jobs only be executed in PRs instead of both P…
ceca4bf 
…Rs and master.

Reason 1: The companion is not merged at the same time as the parent PR
([1](paritytech/parity-processbot#347 (comment))), therefore
the pipeline will fail on master since the companion PR is not yet merged in the other repository.
This scenario is demonstrated by the pipeline of 82cc374.

Reason 2: The job can still fail on master due to a new commit on the companion PR's repository which was merged after `bot merge` happened, as demonstrated by the following scheme:

1. Parent PR is merged
2. Companion PR is updated and set to merge in the future
3. In the meantime a new commit is merged into the companion PR repository's master branch
4. The `check-dependent-*` job runs on master but, due to the new commit, it fails for unrelated reasons

While "Reason 2" can be used as an argument against this PR, in that it would be useful to know if the integration is failing on master, "Reason 1" should be taken care of due to this inherent flaw of the current companion build system design.
@joao-paulo-parity joao-paulo-parity mentioned this issue Dec 23, 2021
Merged
joao-paulo-parity added a commit to paritytech/substrate that referenced this issue Dec 23, 2021
@joao-paulo-parity
make check-dependent-* only be executed in PRs
f2d6ddf 
Make check-dependent-* jobs only be executed in PRs instead of both PRs and master.

Reason 1: The companion is not merged at the same time as the parent PR
([1](paritytech/parity-processbot#347 (comment))), therefore
the pipeline will fail on master since the companion PR is not yet merged in the other repository.
This scenario is demonstrated by the pipeline of 82cc374.

Reason 2: The job can still fail on master due to a new commit on the companion PR's repository which was merged after `bot merge` happened, as demonstrated by the following scheme:

1. Parent PR is merged
2. Companion PR is updated and set to merge in the future
3. In the meantime a new commit is merged into the companion PR repository's master branch
4. The `check-dependent-*` job runs on master but, due to the new commit, it fails for unrelated reasons

While "Reason 2" can be used as an argument against this PR, in that it would be useful to know if the integration is failing on master, "Reason 1" should be taken care of due to this inherent flaw of the current companion build system design.
joao-paulo-parity added a commit to joao-paulo-parity/polkadot that referenced this issue Dec 23, 2021
@joao-paulo-parity
make check-dependent-* only be executed in PRs
e7a2f36 
Make check-dependent-* jobs only be executed in PRs instead of both PRs and
master.

Reason 1: The companion is not merged at the same time as the parent PR
([1](paritytech/parity-processbot#347 (comment))),
therefore the pipeline will fail on master since the companion PR is not yet
merged in the other repository. This scenario is demonstrated by the pipeline
of
paritytech/substrate@82cc374.

Reason 2: The job can still fail on master due to a new commit on the companion
PR's repository which was merged after `bot merge` happened, as demonstrated by
the following scheme:

1. Parent PR is merged
2. Companion PR is updated and set to merge in the future
3. In the meantime a new commit is merged into the companion PR repository's
  master branch
4. The `check-dependent-*` job runs on master but, due to the new commit, it
  fails for unrelated reasons

While "Reason 2" can be used as an argument against this PR, in that it would
be useful to know if the integration is failing on master, "Reason 1" should be
taken care of due to this inherent flaw of the current companion build system
design.
@joao-paulo-parity joao-paulo-parity mentioned this issue Dec 23, 2021
Merged
ordian pushed a commit to paritytech/substrate that referenced this issue Dec 27, 2021
@joao-paulo-parity
make check-dependent-* only be executed in PRs (#10545)
a1fc5cf 
Make check-dependent-* jobs only be executed in PRs instead of both PRs and master.

Reason 1: The companion is not merged at the same time as the parent PR
([1](paritytech/parity-processbot#347 (comment))), therefore
the pipeline will fail on master since the companion PR is not yet merged in the other repository.
This scenario is demonstrated by the pipeline of 82cc374.

Reason 2: The job can still fail on master due to a new commit on the companion PR's repository which was merged after `bot merge` happened, as demonstrated by the following scheme:

1. Parent PR is merged
2. Companion PR is updated and set to merge in the future
3. In the meantime a new commit is merged into the companion PR repository's master branch
4. The `check-dependent-*` job runs on master but, due to the new commit, it fails for unrelated reasons

While "Reason 2" can be used as an argument against this PR, in that it would be useful to know if the integration is failing on master, "Reason 1" should be taken care of due to this inherent flaw of the current companion build system design.
ordian pushed a commit to paritytech/polkadot that referenced this issue Dec 27, 2021
@joao-paulo-parity
make check-dependent-* only be executed in PRs (#4588)
138535e 
Make check-dependent-* jobs only be executed in PRs instead of both PRs and
master.

Reason 1: The companion is not merged at the same time as the parent PR
([1](paritytech/parity-processbot#347 (comment))),
therefore the pipeline will fail on master since the companion PR is not yet
merged in the other repository. This scenario is demonstrated by the pipeline
of
paritytech/substrate@82cc374.

Reason 2: The job can still fail on master due to a new commit on the companion
PR's repository which was merged after `bot merge` happened, as demonstrated by
the following scheme:

1. Parent PR is merged
2. Companion PR is updated and set to merge in the future
3. In the meantime a new commit is merged into the companion PR repository's
  master branch
4. The `check-dependent-*` job runs on master but, due to the new commit, it
  fails for unrelated reasons

While "Reason 2" can be used as an argument against this PR, in that it would
be useful to know if the integration is failing on master, "Reason 1" should be
taken care of due to this inherent flaw of the current companion build system
design.
drahnr pushed a commit to paritytech/polkadot that referenced this issue Jan 4, 2022
@joao-paulo-parity @drahnr
make check-dependent-* only be executed in PRs (#4588)
50465d6 
Make check-dependent-* jobs only be executed in PRs instead of both PRs and
master.

Reason 1: The companion is not merged at the same time as the parent PR
([1](paritytech/parity-processbot#347 (comment))),
therefore the pipeline will fail on master since the companion PR is not yet
merged in the other repository. This scenario is demonstrated by the pipeline
of
paritytech/substrate@82cc374.

Reason 2: The job can still fail on master due to a new commit on the companion
PR's repository which was merged after `bot merge` happened, as demonstrated by
the following scheme:

1. Parent PR is merged
2. Companion PR is updated and set to merge in the future
3. In the meantime a new commit is merged into the companion PR repository's
  master branch
4. The `check-dependent-*` job runs on master but, due to the new commit, it
  fails for unrelated reasons

While "Reason 2" can be used as an argument against this PR, in that it would
be useful to know if the integration is failing on master, "Reason 1" should be
taken care of due to this inherent flaw of the current companion build system
design.
@joao-paulo-parity
Copy link
Contributor

The "It's not possible to meet the minimal approval count of 1" error should be fixed https://github.com/paritytech/parity-processbot/pull/358/files#diff-ee991933fcc804e8dec3619740cb135ffc5a3d19bf55336acecd8b5f05849032R1180. I'll re-open or file a new issue in case it happens again.

The Polkadot update failure is being tracked on #348.

Closing as there's no action left to take here.

Wizdave97 pushed a commit to ComposableFi/polkadot that referenced this issue Feb 3, 2022
@joao-paulo-parity @Wizdave97
make check-dependent-* only be executed in PRs (paritytech#4588)
6a10b5d 
Make check-dependent-* jobs only be executed in PRs instead of both PRs and
master.

Reason 1: The companion is not merged at the same time as the parent PR
([1](paritytech/parity-processbot#347 (comment))),
therefore the pipeline will fail on master since the companion PR is not yet
merged in the other repository. This scenario is demonstrated by the pipeline
of
paritytech/substrate@82cc374.

Reason 2: The job can still fail on master due to a new commit on the companion
PR's repository which was merged after `bot merge` happened, as demonstrated by
the following scheme:

1. Parent PR is merged
2. Companion PR is updated and set to merge in the future
3. In the meantime a new commit is merged into the companion PR repository's
  master branch
4. The `check-dependent-*` job runs on master but, due to the new commit, it
  fails for unrelated reasons

While "Reason 2" can be used as an argument against this PR, in that it would
be useful to know if the integration is failing on master, "Reason 1" should be
taken care of due to this inherent flaw of the current companion build system
design.
grishasobol pushed a commit to gear-tech/substrate that referenced this issue Mar 28, 2022
@joao-paulo-parity @grishasobol
make check-dependent-* only be executed in PRs (paritytech#10545)
517bf0d 
Make check-dependent-* jobs only be executed in PRs instead of both PRs and master.

Reason 1: The companion is not merged at the same time as the parent PR
([1](paritytech/parity-processbot#347 (comment))), therefore
the pipeline will fail on master since the companion PR is not yet merged in the other repository.
This scenario is demonstrated by the pipeline of paritytech@82cc374.

Reason 2: The job can still fail on master due to a new commit on the companion PR's repository which was merged after `bot merge` happened, as demonstrated by the following scheme:

1. Parent PR is merged
2. Companion PR is updated and set to merge in the future
3. In the meantime a new commit is merged into the companion PR repository's master branch
4. The `check-dependent-*` job runs on master but, due to the new commit, it fails for unrelated reasons

While "Reason 2" can be used as an argument against this PR, in that it would be useful to know if the integration is failing on master, "Reason 1" should be taken care of due to this inherent flaw of the current companion build system design.
ark0f pushed a commit to gear-tech/substrate that referenced this issue Feb 27, 2023
@joao-paulo-parity @ark0f
make check-dependent-* only be executed in PRs (paritytech#10545)
11bcad7 
Make check-dependent-* jobs only be executed in PRs instead of both PRs and master.

Reason 1: The companion is not merged at the same time as the parent PR
([1](paritytech/parity-processbot#347 (comment))), therefore
the pipeline will fail on master since the companion PR is not yet merged in the other repository.
This scenario is demonstrated by the pipeline of paritytech@82cc374.

Reason 2: The job can still fail on master due to a new commit on the companion PR's repository which was merged after `bot merge` happened, as demonstrated by the following scheme:

1. Parent PR is merged
2. Companion PR is updated and set to merge in the future
3. In the meantime a new commit is merged into the companion PR repository's master branch
4. The `check-dependent-*` job runs on master but, due to the new commit, it fails for unrelated reasons

While "Reason 2" can be used as an argument against this PR, in that it would be useful to know if the integration is failing on master, "Reason 1" should be taken care of due to this inherent flaw of the current companion build system design.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@koute @ordian @joao-paulo-parity