Skip to content

Pull Request Requirements #1058

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Jun 10, 2025
Merged

Pull Request Requirements #1058

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d722676
Pull Request Requirements
adecaro Apr 24, 2025
b615e57
- fixed Github Project name
adecaro Apr 24, 2025
45701ba
- updated approval policy
adecaro Apr 24, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 17 additions & 2 deletions DEVELOPMENT.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ This document outlines the contribution and development practices to ensure a cl

## 1. Commit Sign-Off Requirement

All commits must be **signed off** to certify that the contributor agrees to the [Developer Certificate of Origin (DCO)](https://developercertificate.org/).
All commits must be **signed off** to certify that the contributor agrees to the [Developer Certificate of Origin (DCO)](https://developercertificate.org/).
This is a legal statement indicating that you wrote the code or have the right to contribute it.

### How to Sign Off a Commit
Expand Down Expand Up @@ -72,8 +72,23 @@ We follow a **linear commit history** to keep the Git log clean and easy to foll
- They make it harder to identify what changes were made.
- They complicate tools that generate changelogs or audit logs.

## 3. Additional Notes
## 3. Pull Request Requirements
Copy link
Contributor

Choose a reason for hiding this comment

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

can you please also mention the PR review process? E.g. is it enough that one of the reviewers approve or must all of them do?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

absolutely.


To maintain clarity and traceability across contributions, all pull requests must adhere to the following:

- **Description**: Every pull request must include a meaningful description outlining the purpose and scope of the change.
- **Labels**: Assign appropriate labels to help with categorization and prioritization.
- **Project Assignment**: The pull request must be added to the **Fabric Token SDK** project with an appropriate status (e.g., “To Do”, “In Progress”, “In Review”, etc.).
- **Associated Issue**: A GitHub Issue must be linked to the pull request.
This should be done via the Development section of the GitHub PR interface (not just mentioned in the description).
Copy link
Contributor

Choose a reason for hiding this comment

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

Test changes also in dependent projects maybe?

This ensures the PR is formally connected to the issue for proper project tracking and status updates.
- **One Approve Policy**: Each PR must receive at least one `approve` from the maintainers of the project before it can be merged.
The approver must ensure that the PR has been reviewed properly and that all CI/CD tests finish successfully.

This ensures that all contributions are tracked, visible on the project board, and aligned with the roadmap.

## 4. Additional Notes
- Name a branch with a concise description of its purpose.
- Use clear, concise commit messages (preferably using [Conventional Commits](https://www.conventionalcommits.org/en/v1.0.0/)).
- Squash commits as needed before submission to keep the history clean.
Copy link
Contributor

Choose a reason for hiding this comment

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

  1. Do we need to do the Squash before rebasing? What is the recommendation?
  2. The default option in the PR ("Squash and Merge") can we change the default to be "Rebase and Merge"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good points:

  1. No, you don't need to squash all your commits before merging into main. The recommendation is to squash commits that have the same message, meaning that they belong to same small unit of changes.
  2. I have disabled the Squash and Merge option.

Copy link
Contributor

Choose a reason for hiding this comment

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

During development we make many small commits/fixes. It makes sense to squash these before merging so that we have less commits. However, ideally these intermediate commits will all be functional versions (the CI will work).

- Open a pull request only after your branch is up-to-date and rebased on `main`.
Loading