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: Check that PR title follows conventional commit prefix #3043

Merged
merged 7 commits into from
Jul 8, 2024

Conversation

untereiner
Copy link
Contributor

@untereiner untereiner commented Mar 15, 2024

The main objective is to be able to release easily. It is easier to compute version numbers based on conventional commits. But constraining every commit message may be too much. To make this process easy I propose to only constrain the merge commit message. It can be easily check via this github action.

The allowed prefixes are:

  • feat: A new feature
  • fix: A bug fix,
  • docs: Documentation only changes,
  • style: Changes that do not affect the meaning of the code (white-space, formatting, missing semi-colons, etc),
  • refactor: A code change that neither fixes a bug nor adds a feature,
  • perf: A code change that improves performance,
  • test: Adding missing tests or correcting existing tests,
  • build: Changes that affect the build system or external dependencies (example scopes: cmake),
  • ci: Changes to our CI configuration files and scripts (example scopes: github),
  • chore: Other changes that don't modify src or test files,
  • revert: Reverts a previous commit,

It is also possible to add your own prefixes…

In case of wrong prefix this stage of the ci will fail and print out the a list of allowed prefixes

https://www.conventionalcommits.org/en/v1.0.0/

@untereiner untereiner self-assigned this Mar 15, 2024
@untereiner untereiner marked this pull request as draft March 15, 2024 17:42
@untereiner untereiner linked an issue Mar 18, 2024 that may be closed by this pull request
@untereiner untereiner marked this pull request as ready for review March 18, 2024 09:59
@untereiner untereiner marked this pull request as draft March 18, 2024 09:59
@untereiner untereiner marked this pull request as ready for review March 18, 2024 12:23
@untereiner untereiner changed the title WIP: deployment wip: deployment Mar 18, 2024
@untereiner untereiner changed the title wip: deployment ci: deployment Mar 18, 2024
@untereiner untereiner marked this pull request as draft March 18, 2024 12:25
@untereiner untereiner changed the title ci: deployment deployment Mar 18, 2024
@untereiner untereiner changed the title deployment ci: deployment Mar 18, 2024
@untereiner untereiner changed the title ci: deployment deployment Mar 18, 2024
@untereiner untereiner changed the title deployment ci: Check that PR title follows conventional commit prefix Mar 18, 2024
@untereiner untereiner marked this pull request as ready for review March 18, 2024 15:10
@TotoGaz
Copy link
Contributor

TotoGaz commented Mar 18, 2024

@rrsettgast @castelletto1 Could you please review this?

@untereiner untereiner added flag: no rebaseline Does not require rebaseline flag: ready for review type: CI Concerns github workflows or generic CI labels Mar 18, 2024
@untereiner untereiner added the ci: run integrated tests Allows to run the integrated tests in GEOS CI label Mar 18, 2024
Copy link

codecov bot commented Mar 18, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 55.74%. Comparing base (aabbe29) to head (4a86fa6).
Report is 87 commits behind head on develop.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #3043      +/-   ##
===========================================
- Coverage    55.75%   55.74%   -0.01%     
===========================================
  Files         1041     1041              
  Lines        88534    88534              
===========================================
- Hits         49358    49356       -2     
- Misses       39176    39178       +2     

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

@rrsettgast
Copy link
Member

Won't this skip the builds on develop?

@untereiner
Copy link
Contributor Author

I added an if to ensure this action runs only at PR state

@rrsettgast
Copy link
Member

Hi @untereiner . Can you explain what situation we are trying to avoid with this change? For instance, you have a check on the PR title prefix, so now every pr must have a prefix from some set of prefixes? What is the set of allowed prefixes?

@untereiner
Copy link
Contributor Author

@rrsettgast I updated the PR text

@rrsettgast
Copy link
Member

perhaps we should have a new prefix: crappy_chore 😬

@untereiner
Copy link
Contributor Author

untereiner commented Mar 19, 2024

What do we do then ? I really think it’s a light way to introduce order.
We can also try to add an action to check changes in the xml or rst tables and request the breaking changes symbol in the title.

@untereiner
Copy link
Contributor Author

@rrsettgast can we give it a try ?

Copy link
Member

@rrsettgast rrsettgast left a comment

Choose a reason for hiding this comment

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

@TotoGaz @wrtobin Can you give feedback on this? I am ok with this, and we can perhaps make custom labels to expand cases like feat to feature...

@TotoGaz
Copy link
Contributor

TotoGaz commented Jun 19, 2024

@TotoGaz @wrtobin Can you give feedback on this? I am ok with this, and we can perhaps make custom labels to expand cases like feat to feature...

Let's try it! If it brings more cons than pros, then we can always remove it 🤷

@untereiner untereiner added the ci: run CUDA builds Allows to triggers (costly) CUDA jobs label Jun 19, 2024
@CusiniM
Copy link
Collaborator

CusiniM commented Jun 30, 2024

isn't this a duplicate of what we were trying to do with labels? If we start using this I think we can get rid of some of the labels.

@untereiner
Copy link
Contributor Author

@CusiniM labels are for CI triage and reviewer information. Here is a first step to automate the process of releasing incremental versions of the code.

@CusiniM
Copy link
Collaborator

CusiniM commented Jul 2, 2024

@CusiniM labels are for CI triage and reviewer information. Here is a first step to automate the process of releasing incremental versions of the code.

yeah some labels have a clear CI function and others are used to triage issues. But, for example, to me having both a label bug and a prefix fix: for the same PR is unnecessary.

@TotoGaz
Copy link
Contributor

TotoGaz commented Jul 2, 2024

yeah some labels have a clear CI function and others are used to triage issues. But, for example, to me having both a label bug and a prefix fix: for the same PR is unnecessary.

Note that one is for PRs, the other for issues?

@CusiniM
Copy link
Collaborator

CusiniM commented Jul 2, 2024

yeah some labels have a clear CI function and others are used to triage issues. But, for example, to me having both a label bug and a prefix fix: for the same PR is unnecessary.

Note that one is for PRs, the other for issues?

well, I think that's how we intended them when we created them in the first place but there is no logic in github for this. It's not like you can define a set of labels only for issues and one only for PRs... In any case, it's not a big deal.

@rrsettgast rrsettgast requested a review from CusiniM July 3, 2024 14:41
@TotoGaz TotoGaz removed their request for review July 5, 2024 17:08
@rrsettgast rrsettgast merged commit f8609a4 into develop Jul 8, 2024
28 checks passed
@rrsettgast rrsettgast deleted the cd/add-semver branch July 8, 2024 21:36
Algiane pushed a commit that referenced this pull request Jul 30, 2024
* add check conventional semantics

* add auth

* only for PR

---------

Co-authored-by: Randolph Settgast <[email protected]>
Co-authored-by: Nicola Castelletto <[email protected]>
Co-authored-by: Matteo Cusini <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci: run CUDA builds Allows to triggers (costly) CUDA jobs ci: run integrated tests Allows to run the integrated tests in GEOS CI flag: no rebaseline Does not require rebaseline flag: ready for review type: CI Concerns github workflows or generic CI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants