-
Notifications
You must be signed in to change notification settings - Fork 59
Docs: Add Conventional Commits Documentation #643
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -117,31 +117,137 @@ fetch master and create these branches for you: | |||||
|
|
||||||
| ### 4. Commit your changes | ||||||
|
|
||||||
| Commit your change. The commit command must include a specific message format: | ||||||
| Commit your changes using the Conventional Commits format. All commits must follow the format described in the [Conventional Commits](#conventional-commits) section below and include proper git trailers for issue tracking. | ||||||
|
|
||||||
| **Basic commit format:** | ||||||
| ```bash | ||||||
| git commit -m "<component>: <change_message> #<issue number>" | ||||||
| git commit -m "<type>(<scope>): <description>" --trailer "Closes: #<issue_number>" | ||||||
| ``` | ||||||
|
|
||||||
| Valid component names are listed in the [__label | ||||||
| list__](https://github.com/rucio/rucio/labels) and are usually specified on the | ||||||
| issue of the change. | ||||||
| **Example:** | ||||||
| ```bash | ||||||
| git commit -m "feat(Core): Add new rule evaluation engine" --trailer "Closes: #1234" | ||||||
| ``` | ||||||
|
|
||||||
| Add additional explanations to the body of the commit, such as motivation for | ||||||
| certain decisions and background information. [Here are some general rules.](https://cbea.ms/git-commit/). | ||||||
|
|
||||||
| If you add a [__github-recognised | ||||||
| keyword__](https://help.github.com/articles/closing-issues-using-keywords/) then | ||||||
| the associated issue can be closed automatically once the pull request is | ||||||
| merged, e.g.: | ||||||
| Using multiple commits is allowed as long as they achieve an independent, | ||||||
| well-defined, change and are well-described. Otherwise multiple commits should | ||||||
| be squashed. | ||||||
|
|
||||||
| ### **Conventional Commits** | ||||||
|
|
||||||
| Rucio enforces the [Conventional Commits](https://www.conventionalcommits.org/) specification to ensure consistent and meaningful commit messages across the project. This is enforced through [commitlint](https://commitlint.js.org/) during CI checks and can be enabled locally via pre-commit hooks. | ||||||
|
|
||||||
| **Commit Message Format:** | ||||||
| ``` | ||||||
| <type>(<scope>): <description> | ||||||
|
|
||||||
| [optional body] | ||||||
|
|
||||||
| [optional footer(s)] | ||||||
| ``` | ||||||
|
|
||||||
| **Rules:** | ||||||
|
|
||||||
| 1. **Type**: Must be one of the following allowed types: | ||||||
| - `feat`: New feature | ||||||
| - `fix`: Bug fix | ||||||
| - `perf`: Code change that improves performance | ||||||
| - `docs`: Documentation only changes | ||||||
| - `style`: Changes that do not affect the meaning of the code | ||||||
| - `refactor`: Code change that neither fixes a bug nor adds a feature | ||||||
| - `test`: Adding missing tests or correcting existing tests | ||||||
| - `build`: Changes that affect the build system or external dependencies | ||||||
| - `ci`: Changes to CI configuration files and scripts | ||||||
| - `chore`: Other changes that don't modify src or test files | ||||||
| - `revert`: Reverts a previous commit | ||||||
|
|
||||||
| 2. **Scope**: Must be one of the predefined Rucio components (PascalCase). The available scopes are: | ||||||
| - `Auth`: Authentication & Authorisation | ||||||
| - `Clients`: Client libraries and tools | ||||||
| - `Consistency`: Consistency checks | ||||||
| - `CI`: Continous Integration | ||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is not a separate component, it's under There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. However, we frequently refactor our CI tooling without modifying any tests. Updates to GitHub Actions, pre-commit hooks, or CI jobs don’t affect the tests directly, so I thought it would make sense to have a separate component dedicated to changes in our GitHub CI infrastructure. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree and I brought this up to the component leads last year, but the conclusion was that we would consider CI under Testing as the component leads would likely be the same for both components anyway. So I'm not sure overall, but I think if we want to maintain the 1:1 mapping between these labels and the components, we should stick to just Testing for now and then we can consider adding a CI component and adding a label for it later on |
||||||
| - `Core`: Core & Internals | ||||||
| - `Database`: Database-related changes | ||||||
| - `DatasetDeletion`: Dataset deletion functionality | ||||||
| - `Deletion`: File deletion functionality | ||||||
| - `DIRAC`: DIRAC integration | ||||||
| - `Docker`: Docker related functionality | ||||||
| - `Documentation`: Documentation updates | ||||||
| - `Kubernetes`: Kubernetes deployment related functionality | ||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Docker and Kubernetes are part of a single component https://rucio.github.io/documentation/component_leads/ - I would also be in favour of maybe just calling it |
||||||
| - `Lifetime`: Life time model processing | ||||||
| - `Messaging`: Messaging system | ||||||
| - `Metadata`: Metadata workflows | ||||||
| - `Monitoring`: Monitoring and observability | ||||||
| - `MultiVO`: Multi-VO functionality | ||||||
| - `Opendata`: Open data functionality | ||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In theory this is not a defined component at the moment https://rucio.github.io/documentation/component_leads/, but I think it makes sense that it would be a separate component. @bari12 what do you think? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There’s already a GitHub label for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I also agree this should be a distinct component. A while back we agreed that the correct name to use would be I have updated the label name to reflect this (to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
I think this would be more consistent with CERN's usage of the term as discussed in the past. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Technically, we would have to decide to add this as a component in Rucio first, which is probably a good idea :-) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am wondering, how would @lobis rebase his |
||||||
| - `Policies`: Policy management | ||||||
| - `Probes`: Probes & Alarms | ||||||
| - `Protocols`: Upload, Download, Deletion protocols | ||||||
| - `Rebalancing`: Data rebalancing | ||||||
| - `Recovery`: Data recovery | ||||||
| - `Replicas`: Replica workflows | ||||||
| - `REST`: REST & API | ||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would vote for |
||||||
| - `Rules`: Replication rules and rule daemons | ||||||
| - `Subscriptions`: Subscription daemon | ||||||
| - `Testing`: Regression and Unit tests | ||||||
| - `Traces`: Monitoring & Traces | ||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Monitoring and traces are part of a single component https://rucio.github.io/documentation/component_leads/ There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we call the component There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think "monitoring" can probably be considered a superset of "traces" so monitoring on its own should be fine, but i like "observability" too. "Telemetry" has a negative connotation imo and it does not reflect what's being done here, so I would avoid using it There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (well, it reflects what's being done here, but we're not doing the kind of telemetry that is associated with negative connotations) |
||||||
| - `Transfers`: Transfer daemons | ||||||
| - `WebUI`: Web user interface | ||||||
|
|
||||||
| **Note**: Any changes to this list should also be applied to the [GitHub labels](https://github.com/rucio/rucio/issues/labels) and the [commitlint config](https://github.com/rucio/rucio/blob/master/commitlint.config.js) | ||||||
|
|
||||||
| 3. **Description**: | ||||||
| - Should not end with a period | ||||||
| - Should be concise but descriptive | ||||||
| - Use imperative mood ("add feature" not "added feature") | ||||||
|
|
||||||
| 4. **Line Length**: Header must not exceed 100 characters to prevent truncation in GitHub UI | ||||||
|
|
||||||
| **Examples:** | ||||||
| ```bash | ||||||
| <component>: <change_message> Fix #<issue number> | ||||||
| feat(Core): Add new rule evaluation engine | ||||||
| fix(DatasetDeletion): Resolve deletion timeout issues | ||||||
| docs(Documentation): Update API documentation | ||||||
| chore(CI): Update Node.js version to 24 | ||||||
| ``` | ||||||
|
|
||||||
| Using multiple commits is allowed as long as they achieve an independent, | ||||||
| well-defined, change and are well-described. Otherwise multiple commits should | ||||||
| be squashed. | ||||||
| ### **Git Trailers** | ||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this should be one header level lower ( There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (also I don't think it should be bold, since there's no other bold headers in this doc) |
||||||
|
|
||||||
| All commits must reference a GitHub issue using git trailers. This ensures proper traceability and automatic issue closure. | ||||||
|
|
||||||
| **Supported Trailers:** | ||||||
| - `Closes: #<issue_number>` - Automatically closes the issue when PR is merged | ||||||
| - `Issue: #<issue_number>` - References an issue without closing it | ||||||
|
|
||||||
| **Adding Git Trailers:** | ||||||
|
|
||||||
| **Method 1: Using git commit command** | ||||||
| ```bash | ||||||
| git commit -m "feat(Core): Add new rule evaluation engine" --trailer "Closes: #1234" | ||||||
| ``` | ||||||
|
|
||||||
| **Method 2: Adding to commit body** | ||||||
| ```bash | ||||||
| git commit -m "feat(Core): Add new rule evaluation engine | ||||||
|
|
||||||
| This commit introduces a new rule evaluation engine that improves | ||||||
| performance and adds support for complex rule conditions. | ||||||
|
|
||||||
| Closes: #1234" | ||||||
| ``` | ||||||
|
|
||||||
| **Complete Example:** | ||||||
| ```bash | ||||||
| feat(Core): Add new rule evaluation engine | ||||||
|
|
||||||
| This commit introduces a new rule evaluation engine that improves | ||||||
| performance and adds support for complex rule conditions. | ||||||
|
|
||||||
| Closes: #1234 | ||||||
| ``` | ||||||
|
|
||||||
| ### 5. Push changes and create a Pull Request | ||||||
|
|
||||||
|
|
||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should be one header level lower (
####) since it's within the### 4. Commit your changesstepThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(also I don't think it should be bold, since there's no other bold headers in this doc)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're technically right. it should be
####since it's nested. However, when I first tried that, it felt a bit off because it didn’t make “conventional commit” stand out as a distinct topic, just regular text. What are your thoughts?