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

Conversation

@joao-paulo-parity
Copy link
Contributor

@joao-paulo-parity joao-paulo-parity commented Feb 7, 2023

@joao-paulo-parity
Copy link
Contributor Author

Per https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/2360018 (note line 127) and https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/2360019 (note line 127) from paritytech/substrate#13324, the patching seems to be happening without issues. The job is failing due to some code-related problem unrelated to diener.

@mordamax
Copy link
Contributor

mordamax commented Feb 7, 2023

@joao-paulo-parity sorry by the provided links the jobs are failed
Could you please also provide an evidence of

  1. regular companion PR
  2. -v PATCH_polkadot=xxx or cumulus case

thanks

@joao-paulo-parity
Copy link
Contributor Author

joao-paulo-parity commented Feb 7, 2023

sorry by the provided links the jobs are failed

Yes, I'm aware. I mentioned that at the end of #506 (comment).

-v PATCH_polkadot=xxx

Why does this matter?

regular companion PR

Why would the flow with a companion PR behave differently w.r.t. diener?

@mordamax
Copy link
Contributor

mordamax commented Feb 7, 2023

Yes, I'm aware. I mentioned that at the end of #506 (comment).

But it's not clear wether it was a wrong patch problem or a code, ideally that should succeed to confirm the evidence of work

-v PATCH_polkadot=xxx

Why does this matter?

The last time we updated a diener, it was working with a simple companion cases, but i didn't with a PATCH_repo. That's why I think it's important to test and make sure

Why would the flow with a companion PR behave differently w.r.t. diener?

Who knows, better know & make sure before roll-out than after

@joao-paulo-parity
Copy link
Contributor Author

But it's not clear wether it was a wrong patch problem or a code, ideally that should succeed to confirm the evidence of work

It was explictly stated that "The job is failing due to some code-related problem unrelated to diener", but alright... I suppose it makes sense to only trust that if the job is actually passing.

The last time we updated a diener, it was working with a simple companion cases, but i didn't with a PATCH_repo. That's why I think it's important to test and make sure

There's no usage of PATCH_something in the companion scripts. Unless I'm missing something? IIRC it was the try-runtime-bot script that had something like that.


Anyways, although I don't think that those extra tests are useful (there should be a standard testing procedure specifically for diener changes depending on what has changed about it), I'll report back here once I have evidence of

  • "No companions" case passing
  • "Polkadot companion" case passing
  • "Cumulus companion" case passing
  • "Polkadot + Cumulus companion" case passing

Copy link
Contributor

@mordamax mordamax left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, looking forward for results!

There's no usage of PATCH_something in the companion scripts.

Aren't we updating it globally, so the companion & command-bot would be affected as well?
if no, which scripts does it affect?

@altaua
Copy link
Contributor

altaua commented Apr 13, 2023

@altaua altaua merged commit 2cc39bf into paritytech:master Apr 13, 2023
altaua pushed a commit to paritytech/pipeline-scripts that referenced this pull request Apr 13, 2023
This reverts commit e3913c5.

No longer needed now that paritytech/scripts#506
has been merged.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

update_substrate_template doesn't replace dependency references correctly

5 participants