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

fix(ci): clean up broken Semantic PR #13499

Merged
merged 2 commits into from
Aug 25, 2022

Conversation

Neudrino
Copy link
Contributor

@Neudrino Neudrino commented Aug 4, 2022

Summary

This is the first POC for the de-tanglement of the different checks. While further improvement might involve composite actions (for the Github commenting) this is seen out of scope for the first iteration.

  • The semantic PR workflow is broken in so far as it has some strange interaction with the cloud-workflow. One effect was, that the semantic PR check would fail, whenever the cloud workflow would fail, even though both should be totally unrelated.
  • One cause of error could be the abuse of files for the propagation of results from previous jobs. While this is speculation, a cleanup to use Github action best practices is worthwhile anyway.
  • With this change the unnecessary bloat and anti-patterns in CI design are removed from the workflow.
    Output variables are totally enough for passing the status on. And this was already implemented.
  • Update: Removes the unnecessary and difficult dependency on comment-pr-on-check-failure.yml.
  • Update: Cleaned up the bloat of the message and only pointed to the documentation as single source of truth.

Test Plan

The workflow is

Additional Information

  • This is a PR from the own repository, in order to be able to test the workflow using on: pull_request
  • UNCHANGED: The notification behaviour is unchanged. I.e. there is a notification for the first (failing) post on the PR. There are no additional notifications on edited messages.

@pull-request-size pull-request-size bot added the size/S Denotes a PR that changes 10-29 lines. label Aug 4, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Aug 4, 2022

Thanks for opening a PR! 💯

A couple initial guidelines

Howto

  • Reviews. The "Reviewers" listed for this PR are the Magma maintainers who will shepherd it.
  • Checks. All required CI checks must pass before merge.
  • Merge. Once approved and passing CI checks, use the ready2merge label to indicate the maintainers can merge your PR.

More info

Please take a moment to read through the Magma project's

If this is your first Magma PR, also consider reading

@github-actions github-actions bot added the component: ci All updates on CI (Jenkins/CircleCi/Github Action) label Aug 4, 2022
@Neudrino Neudrino changed the title fix(ci): clean up broken Semantic PR nonsemantic title - fix(ci): clean up broken Semantic PR Aug 4, 2022
@Neudrino Neudrino force-pushed the ci/clean-up-broken-semantic-pr-workflow branch from cba58ff to 22c4dad Compare August 4, 2022 11:52
@Neudrino Neudrino changed the title nonsemantic title - fix(ci): clean up broken Semantic PR fix(ci): clean up broken Semantic PR Aug 4, 2022
@Neudrino Neudrino changed the title fix(ci): clean up broken Semantic PR broken fix(ci): clean up broken Semantic PR Aug 4, 2022
@Neudrino Neudrino changed the title broken fix(ci): clean up broken Semantic PR Revert fix(ci): clean up broken Semantic PR Aug 4, 2022
@Neudrino Neudrino force-pushed the ci/clean-up-broken-semantic-pr-workflow branch from 22c4dad to f998058 Compare August 4, 2022 12:04
@magma magma deleted a comment from github-actions bot Aug 4, 2022
@Neudrino Neudrino marked this pull request as ready for review August 4, 2022 12:14
@Neudrino Neudrino requested a review from a team as a code owner August 4, 2022 12:14
@Neudrino Neudrino changed the title Revert fix(ci): clean up broken Semantic PR fix(ci): clean up broken Semantic PR Aug 4, 2022
@Neudrino Neudrino self-assigned this Aug 4, 2022
@Neudrino Neudrino marked this pull request as draft August 4, 2022 14:53
@pull-request-size pull-request-size bot added size/M Denotes a PR that changes 30-99 lines. and removed size/S Denotes a PR that changes 10-29 lines. labels Aug 9, 2022
@Neudrino Neudrino changed the title fix(ci): clean up broken Semantic PR broken message - fix(ci): clean up broken Semantic PR Aug 9, 2022
@magma magma deleted a comment from github-actions bot Aug 9, 2022
@Neudrino Neudrino force-pushed the ci/clean-up-broken-semantic-pr-workflow branch 7 times, most recently from 9e9022a to ab519b9 Compare August 9, 2022 13:50
@magma magma deleted a comment from github-actions bot Aug 9, 2022
@Neudrino Neudrino force-pushed the ci/clean-up-broken-semantic-pr-workflow branch 2 times, most recently from ebca969 to 2134943 Compare August 9, 2022 15:45
@Neudrino Neudrino changed the title fix(ci): clean up broken Semantic PR not_conform - fix(ci): clean up broken Semantic PR Aug 9, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Aug 9, 2022

✔️ The Semantic PR check ended with status success. See instructions on formatting your commit and pull request titles.

@Neudrino Neudrino changed the title not_conform - fix(ci): clean up broken Semantic PR fix(ci): clean up broken Semantic PR Aug 9, 2022
@Neudrino Neudrino force-pushed the ci/clean-up-broken-semantic-pr-workflow branch from 2134943 to 61a0de5 Compare August 9, 2022 15:53
@Neudrino Neudrino marked this pull request as ready for review August 9, 2022 16:00
@Neudrino Neudrino enabled auto-merge (squash) August 16, 2022 14:43
@nstng
Copy link
Contributor

nstng commented Aug 18, 2022

As discussed offline, please have a look at

  • triggered comment-pr-on-check-failure.yml is failing iiuc
  • check if the revert case is covered as in comment-pr-on-check-failure.yml
  • check if edit with same text triggers a mail (i.e., check faild after rerun again)

@Neudrino Neudrino force-pushed the ci/clean-up-broken-semantic-pr-workflow branch from 61a0de5 to 016b17f Compare August 22, 2022 09:42
@Neudrino
Copy link
Contributor Author

Neudrino commented Aug 22, 2022

  • triggered comment-pr-on-check-failure.yml is failing iiuc

This was addressed with second commit by removing the trigger.

  • check if the revert case is covered as in comment-pr-on-check-failure.yml

Yes, it is. It does behave as expected and the check and comment step will be skipped in the relevant cases.
Also added to the description.

  • check if edit with same text triggers a mail (i.e., check faild after rerun again)

The behaviour is unchanged here. A notification is sent on the comment creation. There no notification on comment edit. This is Github behaviour and has nothing to do with the actual change here.

@Neudrino Neudrino force-pushed the ci/clean-up-broken-semantic-pr-workflow branch from 016b17f to 061d70c Compare August 22, 2022 11:48
@Neudrino Neudrino requested a review from maxhbr August 22, 2022 13:49
@jheidbrink
Copy link
Contributor

How was the semantic PR broken? The summary says "The semantic PR workflow is broken in so far as it has some strange interaction with the cloud-workflow", but I don't know what those strange interactions are and what doesn't work.

@Neudrino
Copy link
Contributor Author

Neudrino commented Aug 24, 2022

How was the semantic PR broken? The summary says "The semantic PR workflow is broken in so far as it has some strange interaction with the cloud-workflow", but I don't know what those strange interactions are and what doesn't work.

On issue observed was, that the semantic PR check would fail, whenever the cloud workflow would fail. I.e. the semantic PR would depend on the status of the cloud workflow, even though both should be totally unrelated and independent.
(Also extended the PR description.)

@jheidbrink
Copy link
Contributor

Can you link to an example where the semantic PR check failed because of the cloud workflow?

@Neudrino
Copy link
Contributor Author

Neudrino commented Aug 24, 2022

Can you link to an example where the semantic PR check failed because of the cloud workflow?

Link to some random private channel.

Some screenshots are:
image-1
image-2
image-3

(as seen in #13462)

Signed-off-by: Fritz Lehnert <13189449+Neudrino@users.noreply.github.com>
Signed-off-by: Fritz Lehnert <13189449+Neudrino@users.noreply.github.com>
@Neudrino Neudrino force-pushed the ci/clean-up-broken-semantic-pr-workflow branch from 061d70c to 09060c1 Compare August 25, 2022 10:29
@Neudrino
Copy link
Contributor Author

Rebased on master to resolve conflicts from #13696.

@Neudrino Neudrino merged commit f45a9e6 into master Aug 25, 2022
@Neudrino Neudrino deleted the ci/clean-up-broken-semantic-pr-workflow branch August 25, 2022 12:14
@Neudrino
Copy link
Contributor Author

✔️ This PR successfully de-coupled the semantic-pr and cloud-workflow. E.g. see the PR #13715 which has a broken cloud workflow, but the semantic PR is fine.
(Previously the semantic-pr would fail whenever the cloud-workflow fails.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: ci All updates on CI (Jenkins/CircleCi/Github Action) size/M Denotes a PR that changes 30-99 lines.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants