Skip to content

Commit

Permalink
Updated the Code Contribution and Commit Message pages
Browse files Browse the repository at this point in the history
  • Loading branch information
@gtjoseph
gtjoseph committed Oct 17, 2023
1 parent e902c16 commit fdb2190
Show file tree
Hide file tree
Showing 3 changed files with 59 additions and 87 deletions.
2 changes: 1 addition & 1 deletion Makefile
View file
Original file line number Diff line number Diff line change
Expand Up @@ -156,7 +156,7 @@ download-from-job: $(BRANCH_DIR) branch-check
-n documentation-$(subst /,-,$(BRANCH)) -D $(BRANCH_DIR)/source

ifeq ($(BRANCH),)
build: $(BUILD_DIR)/docs dynamic-setup
build: static-setup dynamic-setup
@echo Building to $(SITE_DIR)
@[ ! -f $(BUILD_DIR)/mkdocs.yml ] && \
{ echo "Can't build. '$(BUILD_DIR)/mkdocs.yml' not found" ; exit 1 ; } || :
Expand Down
89 changes: 41 additions & 48 deletions docs/Development/Policies-and-Procedures/Code-Contribution.md
View file
Original file line number Diff line number Diff line change
@@ -1,16 +1,10 @@
---
title: Code Contribution
pageid: 52069715
---

# Overview

# Code Contribution

All code management/contribution/review processes will be handled with [GitHub Asterisk Pull Requests](https://github.com/asterisk/asterisk/pulls) and [GitHub Testsuite Pull Requests](https://github.com/asterisk/testsuite/pulls). Note that Asterisk and Testsuite pull requests must be created in their own repositories.

# Code Contribution Process
## Code Contribution Process

## Install the [GitHub CLI "gh"](https://cli.github.com) tool
### Install the [GitHub CLI "gh"](https://cli.github.com) tool


While not strictly required, using the "[gh](https://cli.github.com)" tool to manage the process will make things much easier. The package is available in most distribution's package management systems as "gh".
Expand All @@ -32,7 +26,7 @@ While not strictly required, using the "[gh](https://cli.github.com)" tool to ma
3. Select "paste an authentication token" as the authentication method.
4. Run `gh auth setup-git` to allow git itself to use the gh authentication method.

## Fork the Repositories
### Fork the Repositories

Contributors must fork the [asterisk/asterisk](https://github.com/asterisk/asterisk) and [asterisk/testsuite](https://github.com/asterisk/testsuite) repositories into their own GitHub account. If you clone the asterisk/asterisk or asterisk/testsuite repositories directly to your development system you will NOT be able to submit pull requests from it. If you have existing repositories cloned from Gerrit, please don't try to re-use the clone by changing the remotes. Similarly, if you happen to already have a fork of the repos in your GitHub account, it's a good idea to delete those forks and re-create them. Starting fresh will ensure you don't have issues later.

Expand All @@ -48,7 +42,7 @@ Git Remotes will automatically be created for both your fork and the upstream re

In each of the clones, run `gh repo set-default`. Select either asterisk/asterisk or asterisk/testsuite as appropriate. They should be the defaults but check anyway. Also run `git config user.email` and `git config user.name` in each of the repos to make sure they're correct. At a minimum, user.email should match one of the emails you've added to your GitHub account.

## Do Work
### Do Work

Checkout the **HIGHEST** VERSION branch to which your work will apply ('master', '20', '18', etc.), update it to match the upstream repo, then push it to your fork.

Expand Down Expand Up @@ -76,38 +70,39 @@ Now make your change and test locally.

[//]: # (end-note)

## Commit
### Commit

Commit messages should follow the guidelines established in [Commit Messages](/Development/Policies-and-Procedures/Commit-Messages). That page will be updated as follows after the cut-over.

* You can use [GitHub Flavored Markdown](https://github.github.com/gfm/) in your commit messages to make them look nicer.
* If there isn't an open GitHub issue for your work, open one now. If there was an existing JIRA issue, you must still open a new GitHub issue.
* To reference an issue, use a `Resolves: #<issueid>` header on a separate line.
* To make users aware of possible breaking changes on update, use an `UpgradeNote: <text>` header starting on a new line and ending with an empty line.
* To make users aware of a new feature or significant fix, use a `UserNote: <text>` header starting on a new line and ending with an empty line.

The new headers create entries near the top of the ChangeLogs.
Commit messages should follow the guidelines established in [Commit Messages](/Development/Policies-and-Procedures/Commit-Messages).

---

Sample Commit Message

```text
app_something: Add some new capability
```
app_foo.c: Add new 'x' argument to the Foo application
app_something has been updated to include new feature "X". To configure,
edit app_something.conf and add an "X = something" to the "general"
section.
The Foo application now has an addition argument 'x' that can manipulate
the output RTP stream of the remote channel by causing it to pause for
a configured amount of time, at a configured interval and a configured
number of times. There's no real use for this other as an example of
how to format a commit message.
Resolves: #456
The code required changes to a number of other modules and is fairly
invasive and poorly written. It also required removing an option from
the existing OldFoo application.
UpgradeNote: The old "X" option in app_something.conf has been renamed to
"Z" to better reflect its true purpose.
Fixes: #666
UserNote: app_something has been updated to include new feature "X".
UserNote: The Foo dialplan application now takes an additional argument
'x(a,b,c)' which will cause the remote channel to pause RTP output for
'a' milliseconds, every 'b' milliseconds, a total of 'c' times.
UpgradeNote: The X argument to the OldFoo application has been removed
and will cause an error if supplied.
```
## Test and check for Cherry-pick-ability

### Test and check for Cherry-pick-ability

This should go without saying but test your change locally to make sure it does what you think it should and that it doesn't break anything else. If it passes and it needs to be cherry-picked to other branches, test cherry-picking now. Create a new branch off the cherry-pick target branch, cherry-pick your change into it then compile and test. If it picks cleanly and passes your tests, you can just delete the branch as you won't be creating additional pull requests for it. If it doesn't apply or pass the tests, you have two options...

Expand All @@ -116,7 +111,7 @@ This should go without saying but test your change locally to make sure it does

You should always use option 1 when possible. Unlike Gerrit, GitHub was never designed to handle pushing the same change to multiple branches. There's no easy way to relate the pull requests and even the GitHub UI doesn't indicate what the target branch is. This makes it labor intensive for us to manage.

## Create a Pull Request
### Create a Pull Request

When you've finished your work and committed, you can create a new pull request by running `gh pr create --fill --base 18`. The `--fill` option sets the pull request description to the same as the commit message and the `--base` option indicates which asterisk branch the pull request is targeted for. This is similar to running `git review 18` to create a new Gerrit review. When prompted where the new branch should be pushed, choose your fork, NOT the upstream repo.

Expand All @@ -125,16 +120,11 @@ You _may_ create a pull request that has more than 1 commit if the commits rep
!!! warning
You must **never** add commits to a pull request that fix issues in earlier commits in that PR.

If you want your change to be automatically cherry-picked to other branches, you'll need to add a comment to your pull request. Head over to <https://github.com/asterisk/asterisk/pulls> and open your PR. Add a comment with a `cherry-pick-to: <branch>"` header line for each branch. For example, if the PR is against the master branch and you want it cherry-picked down to 20 and 18...

---

PR Cherry-Pick Request comment example
If you want your change to be automatically cherry-picked to other branches, you'll need to add a comment to your pull request. Head over to <https://github.com/asterisk/asterisk/pulls> and open your PR. Add a comment with a `cherry-pick-to: <branch>"` header line for each branch. For example, if the PR is against the master branch and you want it cherry-picked down to 20 and 18, add a comment with the following:

```text
cherry-pick-to: 20
cherry-pick-to: 18
```

Each branch must be on a separate line and don't put anything else in the comment. When all the PR tests and checks have passed, an Asterisk Core developer will trigger the cherry-pick test process which will look for that comment. If the commit can't be cherry-picked cleanly to the branches you indicated or the tests fail, none of the commits will be merged. This is why it's important for you to make sure your commit cherry-picks cleanly before submitting the first pull request.
Expand All @@ -146,36 +136,39 @@ If you don't need your PR automatically cherry-picked, please add a comment stat

[//]: # (end-note)

!!! warning
Don't add the cherry-pick-to lines to the commit message or the PR description. They're only searched for in PR comments.

!!! warning
**If you change your mind and don't want your PR automatically cherry-picked, edit the comment and replace the "cherry-pick-to" lines with a single `cherry-pick-to: none` line** Don't use formatting or other means to say "nevermind". The automation might not understand.

[//]: # (end-warning)

# Pull Request Review Process
## Pull Request Review Process

As with Gerrit reviews, a new PR triggers a set of tests and checks. If you browse to your PR and scroll to the bottom, you'll see the status of those checks listed. There are some differences to Gerrit however.

## New Contributor License Agreement
### New Contributor License Agreement

Every contributor will be required to sign a new Contributor License Agreement before their first PR can be merged. One of the PR checks will be "license/cla" which looks like this...

![](image2023-4-17-14:4:2.png)

which indicates that you haven't signed it yet. Click the "Details" link to be taken to the page that allows you to fill out the form and sign. Acceptance is automatic so there should be no delay and you only have to do this once. YOUR PR CANNOT BE MERGED UNTIL THIS CHECK IS COMPLETED.

## Automated Tests
### Automated Tests

GitHub gives us access to more resources for testing than we've ever had so instead of running the Unit tests at PR submission and the Gate/Testsuite tests when the change has been approved, we run both the Unit and Gate/Testsuite tests immediately upon submission. The Unit tests run as a single job/check but the Gates are broken up into multiple jobs/checks so they can be run in parallel. When each check is completed, a comment will be added to the PR with the result and each check will have it's own line in the checks summary a the bottom of the PR. All checks must pass or be deemed "false alarms" before a PR can be merged.

## Reviewing a change
### Reviewing a change

GitHub has two types of Pull Request comments.

### General Comments
#### General Comments

These are comments you leave on the main PR page. They are just that...comments. They have no bearing on whether the PR can be merged. If you have general comments or questions about a PR, this is where you leave them. The Gerrit equivalent is clicking the "REPLY" button at the top of the review and leaving a comment without changing your vote.

### Review Comments
#### Review Comments

These are comments you have about the code itself. These are left by clicking on the 'Files changed" tab at the top of the PR, then...

Expand All @@ -189,7 +182,7 @@ These are comments you have about the code itself. These are left by clicking o

[//]: # (end-note)

## Address Review Comments and Test Failures
### Address Review Comments and Test Failures

If you need to make code changes to address comments or failures, the process is much like it is with Gerrit...

Expand All @@ -200,16 +193,16 @@ If you need to make code changes to address comments or failures, the process is
This will force push the commit to your fork first, then update the PR with the new commit and restart the testing process.

!!! warning
Unlike Gerrit, GitHub allows you to have multiple commits for a pull request but it was intended to allow a PR to be broken up into multiple logical chunks, not to address review comments. Using multiple commits to address review comments will make the commit history messy and confusing. Please amend and force push for them otherwise the PR will be rejected.
Unlike Gerrit, GitHub allows you to have multiple commits for a pull request but it was intended to allow a PR to be broken up into multiple logical chunks, not to address review comments. Using multiple commits to address review comments will make the commit history messy and confusing. Please amend and force push otherwise the PR will be rejected.

[//]: # (end-warning)


## Cherry-Pick Tests
### Cherry-Pick Tests

When an Asterisk Core Team member believes the PR is ready, they'll add a `cherry-pick-test` label to the PR that will jobs to run that check that the cherry-pick applies cleanly to the other branches and run the same automated tests that ran for the original PR. These tests must pass (or be deemed false alarms) for the PR to be eligible for merging.

## Merge
### Merge

When an Asterisk Core Team member believes the PR is ready for merging, they'll approve the merge which will cause the original PR to merge into its target branch and cause the change to be cherry-picked into each of the cherry-pick target branches.

Loading

0 comments on commit fdb2190

Please sign in to comment.