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

[CI] Fix SemVer check base commit #5361

Merged
merged 33 commits into from
Aug 26, 2024
Merged

[CI] Fix SemVer check base commit #5361

merged 33 commits into from
Aug 26, 2024

Conversation

ggwpez
Copy link
Member

@ggwpez ggwpez commented Aug 14, 2024

After seeing some cases of reported changes that did not happen by the merge request proposer (like #5339), it became clear that this is probably the issue.
The base commit of the SemVer check CI is currently using the latest master commit, instead of the master commit at the time when the MR was created.

Trying to get the correct base commit now. For this to be debugged, i have to wait until another MR is merged into master.

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Copy link

Command failed ❌

Run by @ggwpez for Command PrDoc failed. See logs here.

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
@ggwpez ggwpez marked this pull request as ready for review August 22, 2024 12:59
@ggwpez ggwpez requested review from a team as code owners August 22, 2024 12:59
Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
@ggwpez ggwpez added the R0-silent Changes should not be mentioned in any release notes label Aug 22, 2024
@Morganamilo
Copy link
Contributor

I think the best solution here may be to just remove the old branch creation and call parity publish with --since HEAD^1. Because we checkout the merged version of the PR, the state of HEAD will always be a merge commit between the PR branch and the base branch. Using HEAD^1 gets us the branch that the PR was merged into. This should stop any desync possibilities.

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
@paritytech-cicd-pr
Copy link

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: test-linux-stable-int
Logs: https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/7118698

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
@bkchr
Copy link
Member

bkchr commented Aug 26, 2024

Because we checkout the merged version of the PR,

Are we doing this? Wouldn't this require a merge commit? Maybe I'm missing something. I tried to read the checkout action documentation, but couldn't see this mentioned.

A solution to this problem could also be to merge master into the pr before running the check.

@ggwpez
Copy link
Member Author

ggwpez commented Aug 26, 2024

Are we doing this? Wouldn't this require a merge commit? Maybe I'm missing something. I tried to read the checkout action documentation, but couldn't see this mentioned.

The checkout action always creates a local merge commit and then runs on that. I dont find the docs either, but there is a SE post and from looking at the CI output, it always seems to merge it into master first:

Screenshot 2024-08-26 at 12 58 23

This commit 166 does not exist exist on my branch. But the 36f is the last on my branch and the 475 is on master:
Screenshot 2024-08-26 at 12 58 35

master:
Screenshot 2024-08-26 at 12 58 46

Ultimately i only found this when searching how other repos do this, for example here.

A solution to this problem could also be to merge master into the pr before running the check.

Yea this works, but is a bit annoying and produces many false-positives.

@bkchr
Copy link
Member

bkchr commented Aug 26, 2024

@Morganamilo can you approve?

@ggwpez ggwpez added this pull request to the merge queue Aug 26, 2024
Merged via the queue into master with commit b34d4a0 Aug 26, 2024
189 of 190 checks passed
@ggwpez ggwpez deleted the oty-prdoc-check branch August 26, 2024 20:24
ordian added a commit that referenced this pull request Aug 27, 2024
* master: (36 commits)
  Bump the ci_dependencies group across 1 directory with 2 updates (#5401)
  Remove deprecated calls in cumulus-parachain-system (#5439)
  Make the PR template a default for new PRs (#5462)
  Only log the propagating transactions when they are not empty (#5424)
  [CI] Fix SemVer check base commit (#5361)
  Sync status refactoring (#5450)
  Add build options to the srtool build step (#4956)
  `MaybeConsideration` extension trait for `Consideration` (#5384)
  Skip slot before creating inherent data providers during major sync (#5344)
  Add symlinks for code of conduct and contribution guidelines (#5447)
  pallet-collator-selection: correctly register weight in `new_session` (#5430)
  Derive `Clone` on `EncodableOpaqueLeaf` (#5442)
  Moving `Find FAIL-CI` check to GHA (#5377)
  Remove panic, as proof is invalid. (#5427)
  Reactive syncing metrics (#5410)
  [bridges] Prune messages from confirmation tx body, not from the on_idle (#5006)
  Change the chain to Rococo in the parachain template Zombienet config (#5279)
  Improve the appearance of crates on `crates.io` (#5243)
  Add initial version of `pallet_revive` (#5293)
  Update OpenZeppelin template documentation (#5398)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
R0-silent Changes should not be mentioned in any release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants