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

Cancel CI PR jobs that are in progress with new changes, add skip_changelog label to overrides #1057

Merged
merged 2 commits into from Jan 30, 2022

Conversation

ml-evs
Copy link
Member

@ml-evs ml-evs commented Jan 26, 2022

As above. This PR will cancel workflows from running when new changes are pushed to a PR.

@ml-evs ml-evs added the CI Continuous Integration - GitHub Actions issues (NOT related to the repository Action) label Jan 26, 2022
@codecov
Copy link

codecov bot commented Jan 26, 2022

Codecov Report

Merging #1057 (86e6973) into master (b473382) will not change coverage.
The diff coverage is n/a.

❗ Current head 86e6973 differs from pull request most recent head 4da9bdd. Consider uploading reports for the commit 4da9bdd to get more accurate results
Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1057   +/-   ##
=======================================
  Coverage   92.90%   92.90%           
=======================================
  Files          67       67           
  Lines        3790     3790           
=======================================
  Hits         3521     3521           
  Misses        269      269           
Flag Coverage Δ
project 92.90% <ø> (ø)
validator 92.90% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
optimade/validator/utils.py 94.63% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b473382...4da9bdd. Read the comment docs.

@CasperWA
Copy link
Member

This confuses me. You are now excluding all PRs with a CI label in the changelog and also changing the way CI workflows are run? It seems the PR should only be for the latter?

@ml-evs
Copy link
Member Author

ml-evs commented Jan 26, 2022

This confuses me. You are now excluding all PRs with a CI label in the changelog and also changing the way CI workflows are run? It seems the PR should only be for the latter?

Yes, I thought the CI changelog change is a minor one so added it alongside this CI tweak. Another option would be to add a special label "skip_changelog" that we can use to manually override the rule. Do you think CI changes should be in the changelog?

@CasperWA
Copy link
Member

Yes, I thought the CI changelog change is a minor one so added it alongside this CI tweak. Another option would be to add a special label "skip_changelog" that we can use to manually override the rule. Do you think CI changes should be in the changelog?

A skip_changelog label sounds like the perfect solution for this.
While CI changes are not directly related to the package, I don't want to hide the work done to automate and implement good testing and more from the changelog. I think it's a big part of what makes the repository strong; solid CI/CD and should therefore be credited and included in the changelog.

@ml-evs
Copy link
Member Author

ml-evs commented Jan 26, 2022

Yes, I thought the CI changelog change is a minor one so added it alongside this CI tweak. Another option would be to add a special label "skip_changelog" that we can use to manually override the rule. Do you think CI changes should be in the changelog?

A skip_changelog label sounds like the perfect solution for this. While CI changes are not directly related to the package, I don't want to hide the work done to automate and implement good testing and more from the changelog. I think it's a big part of what makes the repository strong; solid CI/CD and should therefore be credited and included in the changelog.

Fair point, I guess we can push the quibbling to whether each individual PR should have a skip_changelog tag 🙃 I will do it for this one at least...

@ml-evs ml-evs force-pushed the ml-evs/cancel_in_progress_CI branch from 86e6973 to 6a69a8a Compare January 26, 2022 15:57
@CasperWA
Copy link
Member

Yes, I thought the CI changelog change is a minor one so added it alongside this CI tweak. Another option would be to add a special label "skip_changelog" that we can use to manually override the rule. Do you think CI changes should be in the changelog?

A skip_changelog label sounds like the perfect solution for this. While CI changes are not directly related to the package, I don't want to hide the work done to automate and implement good testing and more from the changelog. I think it's a big part of what makes the repository strong; solid CI/CD and should therefore be credited and included in the changelog.

Fair point, I guess we can push the quibbling to whether each individual PR should have a skip_changelog tag 🙃 I will do it for this one at least...

Not sure that should be the case either. I like a chunky changelog 😅 Even if each line represents a few character changes in the code base... Depending on the characters changed the PR may be significant or not ;)

@ml-evs ml-evs force-pushed the ml-evs/cancel_in_progress_CI branch from 6a69a8a to 779f591 Compare January 26, 2022 15:58
@ml-evs ml-evs added the skip_changelog Use this label to omit this PR/issue from the CHANGELOG. label Jan 26, 2022
@ml-evs
Copy link
Member Author

ml-evs commented Jan 26, 2022

Not sure that should be the case either. I like a chunky changelog 😅 Even if each line represents a few character changes in the code base... Depending on the characters changed the PR may be significant or not ;)

Hmm, I guess this is an important point then. To me, our changelog is user-facing (where user could include anyone who depends on this library) and not developer facing. But I guess your issue with the "Update Dependency" entries was more the repetition (and lack of info)? I'm not overly fussed either way, if skip_changelog works for you then we can just merge this and use it sparingly (for cases where we mess up, for example 🙃)

@ml-evs ml-evs force-pushed the ml-evs/cancel_in_progress_CI branch from 779f591 to 4da9bdd Compare January 26, 2022 16:02
@ml-evs ml-evs removed the skip_changelog Use this label to omit this PR/issue from the CHANGELOG. label Jan 26, 2022
@ml-evs ml-evs changed the title Cancel CI PR jobs that are in progress with new changes, exclude CI from changelog Cancel CI PR jobs that are in progress with new changes, add skip_changelog label to overrides Jan 26, 2022
@ml-evs
Copy link
Member Author

ml-evs commented Jan 26, 2022

(The cancel in progress thing works nicely though, should save us some build minutes across here and the dashboard builds)

Copy link
Contributor

@JPBergsma JPBergsma left a comment

Choose a reason for hiding this comment

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

I did not see anything wrong with this PR.
I do not know much about GitHub actions, though, so a mistake could easily have slipped past me.

@ml-evs
Copy link
Member Author

ml-evs commented Jan 30, 2022

I think @CasperWA and I reached agreement on the labelling, so I will merge this now

@ml-evs ml-evs merged commit 98948f3 into master Jan 30, 2022
@ml-evs ml-evs deleted the ml-evs/cancel_in_progress_CI branch January 30, 2022 22:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI Continuous Integration - GitHub Actions issues (NOT related to the repository Action)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants