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): err due to update-node-dist being canceled #6801

Merged
merged 1 commit into from
Dec 19, 2024

Conversation

max-nextcloud
Copy link
Collaborator

The update-node-dist workflow pushes a new commit to the branch it runs on.
This commit triggers the workflow again.
Prevent the second workflow from canceling the first
just before the first would have finished.

Every time the workflow is canceled it sends out a notification.

Without cancel-in-progress there will be at most one workflow running and one pending.
Most of the time the pending workflow will be the one that resulted from the commit pushed by the running workflow.
If two PRs are merged quickly after each other
first run will be finished even though it was not needed.
However this will happen rarely and will at most waste a few minutes of CI runtime.

The `update-node-dist` workflow pushes a new commit to the branch it runs on.
This commit triggers the workflow again.
Prevent the second workflow from canceling the first
just before the first would have finished.

Every time the workflow is canceled it sends out a notification.

Without `cancel-in-progress` there will be at most one workflow running and one pending.

Most of the time the pending workflow will be the one that resulted from the commit pushed by the running workflow.

If two PRs are merged quickly after each other
first run will be finished even though it was not needed.
However this will happen rarely and will at most waste a few minutes of CI runtime.

Signed-off-by: Max <[email protected]>
Copy link

codecov bot commented Dec 19, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 46.76%. Comparing base (702ef89) to head (92da92f).
Report is 2 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #6801   +/-   ##
=======================================
  Coverage   46.76%   46.76%           
=======================================
  Files         748      748           
  Lines       34163    34163           
  Branches     1242     1242           
=======================================
  Hits        15976    15976           
  Misses      17565    17565           
  Partials      622      622           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@mejo- mejo- left a comment

Choose a reason for hiding this comment

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

Mhh, I'm not sure whether this is the right approach. If your assumption is correct, that the commit by update-node-dist triggers the workflow again, then removing cancel-in-progress would mean an endless chain of actions being activated by a merge, no?

I wonder whether something like https://docs.github.com/en/actions/writing-workflows/choosing-when-your-workflow-runs/events-that-trigger-workflows#running-your-pull_request_target-workflow-when-a-pull-request-merges would be better.

@max-nextcloud
Copy link
Collaborator Author

then removing cancel-in-progress would mean an endless chain of actions being activated by a merge, no?

No. The actual build run creates a new commit and pushes it. The second run builds again, compares the output and notices there's nothing to push. That would stay the same as it currently is. It's just that the second run would not cancel the first one after it already pushed its commit anymore.

Copy link
Member

@mejo- mejo- left a comment

Choose a reason for hiding this comment

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

Aha, thanks for the explanation, makes sense. And thanks for looking into this!

@max-nextcloud
Copy link
Collaborator Author

max-nextcloud commented Dec 19, 2024

I wonder whether something like https://docs.github.com/en/actions/writing-workflows/choosing-when-your-workflow-runs/events-that-trigger-workflows#running-your-pull_request_target-workflow-when-a-pull-request-merges would be better.

I was looking into something similar but this is even closer to it. However we also need to rebuild the js assets after the transifex changes have been pushed and they are pushed directly to master.

I wonder if we could do something like this:

on:
  pull_request_target:
    types:
      - closed
    branches:
      - main
      - stable30
      - stable29
      - stable28
  push:
    paths:
      - l10n
    branches:
      - main
      - stable30
      - stable29
      - stable28

@max-nextcloud max-nextcloud merged commit c76f190 into main Dec 19, 2024
68 checks passed
@max-nextcloud max-nextcloud deleted the fix/update-node-dist-error branch December 19, 2024 13:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants