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: remove running CI for superseded commits #8213

Merged
merged 19 commits into from
Oct 27, 2022

Conversation

dblythy
Copy link
Member

@dblythy dblythy commented Oct 6, 2022

New Pull Request Checklist

Issue Description

Currently, workflows continue to run even after new pushes have been commited. This can be more efficient and can save workflow billing

Related issue: n/a

Approach

Uses concurrency to cancel existing runs

TODOs before merging

  • A changelog entry is created automatically using the pull request title (do not manually add a changelog entry)

@parse-github-assistant
Copy link

parse-github-assistant bot commented Oct 6, 2022

Thanks for opening this pull request!

  • ❌ Please link an issue that describes the reason for this pull request, otherwise your pull request will be closed. Make sure to write it as Related issue: #123 in the PR description, so I can recognize it.

@dblythy
Copy link
Member Author

dblythy commented Oct 6, 2022

As a POC you can see these workflow were both cancelled automatically as a new commit was created

https://github.com/parse-community/parse-server/actions/runs/3196813578
https://github.com/parse-community/parse-server/actions/runs/3196815216

@codecov
Copy link

codecov bot commented Oct 6, 2022

Codecov Report

Base: 93.61% // Head: 94.13% // Increases project coverage by +0.51% 🎉

Coverage data is based on head (5a57c74) compared to base (28f0d26).
Patch has no changes to coverable lines.

Additional details and impacted files
@@            Coverage Diff             @@
##            alpha    #8213      +/-   ##
==========================================
+ Coverage   93.61%   94.13%   +0.51%     
==========================================
  Files         182      182              
  Lines       13771    13771              
==========================================
+ Hits        12892    12963      +71     
+ Misses        879      808      -71     
Impacted Files Coverage Δ
src/Adapters/Storage/Mongo/MongoStorageAdapter.js 93.19% <0.00%> (+0.21%) ⬆️
src/RestWrite.js 94.78% <0.00%> (+0.29%) ⬆️
src/Adapters/Files/GridFSBucketAdapter.js 94.16% <0.00%> (+0.72%) ⬆️
src/ParseServerRESTController.js 98.48% <0.00%> (+1.51%) ⬆️
src/batch.js 94.73% <0.00%> (+1.75%) ⬆️
src/GraphQL/transformers/mutation.js 97.16% <0.00%> (+9.43%) ⬆️
src/GraphQL/loaders/filesMutations.js 79.31% <0.00%> (+41.37%) ⬆️
src/Adapters/Cache/RedisCacheAdapter.js 87.71% <0.00%> (+75.43%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@mtrezza
Copy link
Member

mtrezza commented Oct 6, 2022

Amazing, I wanted to look into that for months and here is the fix - very nice! This should be added to every CI workflow in our 6 major repos.

This is only within the workflow scope, right? So this setting doesn't change the behavior of any other workflows.

@mtrezza mtrezza changed the title ci: add concurrency depending on PR ci: remove running CI for superseded commits Oct 6, 2022
@dblythy
Copy link
Member Author

dblythy commented Oct 6, 2022

GitHub actions will cancel any running test that has the same group value. I've set it here to the PR's user + branch name, so here it's dblythy-concurrent, so the only runs that will be cancelled are those that match the group.

@mtrezza
Copy link
Member

mtrezza commented Oct 6, 2022

Could you restrict this on a branch + workflow name basis? Otherwise this logic may interfere with other workflows that run on the same branch should they implement a similar logic.

Maybe there is something in the context docs that can be used.

@dblythy
Copy link
Member Author

dblythy commented Oct 7, 2022

I tried github.run-name but it is empty. Do you have any suggestions?

@mtrezza
Copy link
Member

mtrezza commented Oct 7, 2022

Could this be a bug or the reference incorrect? Because name should always have a value:

The name of your workflow. GitHub displays the names of your workflows on your repository's "Actions" tab. If you omit name, GitHub sets it to the workflow file path relative to the root of the repository. Maybe open a GitHub support ticket?

You could also look into the env vars, maybe GITHUB_ACTION or GITHUB_ACTION_REPOSITORY?

@dblythy
Copy link
Member Author

dblythy commented Oct 12, 2022

Could you restrict this on a branch + workflow name basis?

Could you give an example for what the output should be? With actor + branch, the concurrency key is "dblythy-concurrent", meaning any other action with the concurrency key of "dblythy-concurrent" is cancelled as well

@mtrezza
Copy link
Member

mtrezza commented Oct 12, 2022

I think the output should be the specific workflow. We want to avoid that one workflow cancels another workflow if someone in the future just copy/pastes the cancellation logic.

For example: we have ci.yaml and when a new run is triggered because of a new commit in the PR, it should cancel only any previous run of ci.yaml. Maybe one way to achieve this is to add an env variable (or workflow name if that property exists) at the top of the workflow file, and use that as a condition to cancel all workflows with that property. But that would require manually changing the name and copy/pasting would again cause issues. So it should be an ID that is unique to the workflow (file) that triggered the run.

@dblythy
Copy link
Member Author

dblythy commented Oct 13, 2022

I think concurrency only exists across the whole workflow, I’m not sure you can specify it at a per job basis

1 similar comment
@dblythy
Copy link
Member Author

dblythy commented Oct 13, 2022

I think concurrency only exists across the whole workflow, I’m not sure you can specify it at a per job basis

@mtrezza
Copy link
Member

mtrezza commented Oct 15, 2022

With "job" I meant "workflow", sorry for the confusion. We'd want to make sure that this cancellation logic is only considered for the specific workflow; if that is always the case anyway and a workflow cannot cancel another workflow, then we are good. if not, maybe you could open a support ticket with GitHub and ask how to do that.

Moumouls
Moumouls previously approved these changes Oct 16, 2022
Copy link
Member

@Moumouls Moumouls left a comment

Choose a reason for hiding this comment

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

Ohhh really interesting @dblythy , i didn't know that Github Action are able to handle this use case.

You are right it's useless to let the CI run on new commit push.

@mtrezza
Copy link
Member

mtrezza commented Oct 16, 2022

@Moumouls You approved this even though there is an ongoing discussion - what is your opinion regarding the issue?

@mtrezza
Copy link
Member

mtrezza commented Oct 27, 2022

I think this section in the docs addresses the issue I was referring to:

If you have multiple workflows in the same repository, concurrency group names must be unique across workflows to avoid canceling in-progress jobs or runs from other workflows. Otherwise, any previously in-progress or pending job will be canceled, regardless of the workflow.

And they provide a solution:

To only cancel in-progress runs of the same workflow, you can use the github.workflow property to build the concurrency group:

concurrency: 
  group: ${{ github.workflow }}-${{ github.ref }}
  cancel-in-progress: true

Could we try that? It would be interesting to see the output of ${{ github.workflow }}-${{ github.ref }} for the workflow.

@mtrezza
Copy link
Member

mtrezza commented Oct 27, 2022

The workflow gets canceled with:

Canceling since a higher priority waiting request for 'ci-refs/pull/8213/merge' exists

  • github.workflow = ci which is the name of the workflow as defined at name: ci
  • github.ref = refs/pull/8213/merge is unique per PR

This looks good, so it's on a per workflow per PR basis. What do you think?

@dblythy
Copy link
Member Author

dblythy commented Oct 27, 2022

Nice find! Sounds good to me

Copy link
Member

@mtrezza mtrezza left a comment

Choose a reason for hiding this comment

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

Looks good!

@mtrezza mtrezza merged commit c41e5fc into parse-community:alpha Oct 27, 2022
@parseplatformorg
Copy link
Contributor

🎉 This change has been released in version 5.3.0-alpha.32

@parseplatformorg parseplatformorg added the state:released-alpha Released as alpha version label Oct 29, 2022
@dblythy dblythy deleted the concurrent branch October 29, 2022 19:54
@parseplatformorg
Copy link
Contributor

🎉 This change has been released in version 5.4.0-beta.1

@parseplatformorg parseplatformorg added the state:released-beta Released as beta version label Oct 29, 2022
@parseplatformorg
Copy link
Contributor

🎉 This change has been released in version 5.4.0-alpha.1

@parseplatformorg
Copy link
Contributor

🎉 This change has been released in version 5.4.0

@parseplatformorg parseplatformorg added the state:released Released as stable version label Nov 19, 2022
@parseplatformorg
Copy link
Contributor

🎉 This change has been released in version 5.4.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
state:released Released as stable version state:released-5.x.x Released as LTS version state:released-alpha Released as alpha version state:released-beta Released as beta version
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants