All non-trivial pull requests should be reviewed and merged by someone other than the author. A pull request that touches code is never trivial, but one that fixes a typo in the documentation probably is.
Trivial updates, such as docs updates, do not require a logged issue.
Outside of very trivial issues like typo’s, if a PR is opened without an associated issue, the PR description should then briefly explain why the PR is needed or what led to its creation.
All team members are encouraged to review community contributions. However, each PR should have a single accountable reviewer, who is also approved as a CODEOWNER. That reviewer can ask others in the team for feedback but they are solely accountable for the merge/approval decision.
If you are assigned as primary reviewer and do not feel confident in your ability to approve and merge, it is your responsibility to either (a) request assist from a team member on specific parts of the code, or (b) ask another team member to take the role of primary reviewer.
PR approvals are set (on a per-repo basis) to not use the option to Remove all approvals when commits are added to source branch
. This means approvals are “sticky” and can be requested any time during the review cycle. This also means it is the Merger’s responsibility to check commit history and raise an alarm on any regressions or other concerns introduced after another team member granted their approval.
Security Note: In most cases, the closing “post-approval” tasks should be cosmetic - such as docs, linting, and changelog updates - but team members should nevertheless be on the lookout for any regressions or malicious-looking code that may have been added after approvals are given and before the Merge is applied. (If in doubt, please request a repeat-review from other approvers on the PR.)
Team authored PRs may be reviewed by any other team member, but should also be approved by a code owner, as described below.
For community contributions, the community contributor should indicate readiness to merge and the core team member (primary reviewer) will approve the PR and also perform the merge.
We aim to be responsive in all community contributed PRs, as a sign of respect for the community members’ contributed time and effort.
The first team member to review should assign themselves to the review and check the following are present:
Note: If not comfortable being primary approver, due to either time constraints or subject matter expertise, the first reviewer should request review by tagging another team member.
Awaiting Action::Author
label. Sparingly and with due respect, we may ping a contributor on Slack (in DM or in the #contributing
channel) to notify them of pending action on their side.Occasionally we need to help a contributor get their PR completed by contributing back to their fork. The GitLab-provided instructions are incorrect for this process. Please use the following:
According to the docs:
Only pull request authors can give upstream repository maintainers, or those with push access to the upstream repository, permission to make commits to their pull request’s compare branch in a user-owned fork.
Note that this only works for pull requests that are authored by a user-owned fork but not if it’s an organization-owned fork.
export FORK_ORG_NAME=name-of-user
export FORK_REPO_NAME=meltano
export FORK_BRANCH_NAME=name-of-branch
export TARGET_BRANCH_NAME=main
# Add the remote
git remote add $FORK_ORG_NAME "https://github.com/$FORK_ORG_NAME/$FORK_REPO_NAME.git"
# Fetch the branch refs
git fetch $FORK_ORG_NAME
# Checkout the branch
git checkout -b "$FORK_ORG_NAME-$FORK_BRANCH_NAME" --track "$FORK_ORG_NAME/$FORK_BRANCH_NAME"
If you need to pull new commits later on (before deleting the remote):
git pull $FORK_ORG_NAME $FORK_BRANCH_NAME
Optionally, merge in the latest from master and resolve conflicts:
git merge "origin/$TARGET_BRANCH_NAME"
Then make any other changes needed. For example, commonly we need to re-lock the python library refs:
poetry lock
git push $FORK_ORG_NAME "$FORK_ORG_NAME-$FORK_BRANCH_NAME:$FORK_BRANCH_NAME"
Remove the local remote ref and branch:
git checkout $TARGET_BRANCH_NAME
git branch -D "$FORK_ORG_NAME-$FORK_BRANCH_NAME"
git remote remove $FORK_ORG_NAME
For our core repos we use a pattern of Primary/Fallback ownership, where each area of the codebase has a designated primary and secondary owners. Approval is required for both Community-Contributed PRs and Team-Authored PRs from one of these individuals. This can be requested either when the PR foundation is in place or as a “final check”. The final approval from the Primary code owner should generally be requested after the PR is otherwise “clean” - and after known action items and questions are called out in the text of the PR. In the scenario where the primary code owner is also an author they must obtain approval from the “fallback” owner.
The code owners for Meltano and the SDK are specified in their respective CODEOWNERS
files:
SDK - CODEOWNERS
Meltano - CODEOWNERS
As we grow and the complexity of the various code base increases, we will appoint additional code owners to specific subject areas as needed.
For any Pulumi and IAC resources which don’t already have a CODEOWNERS
ruleset, the following approval model should apply:
Whenever spec changes/updates are introduced during the process of development, and/or whenever a large impactful feature is being implemented, please add @seth-meltano
and @tayloramurphy
as required approvers.
All EM/PM approval requests should be paired with a comment tagging one or both, specifically addressing the details of where spec has changed and/or which components or choices are needing approval.
There are three types of EM/PM approval requests:
As experts catch issues in PRs that the original reviewers did not, we will update this section and the Contributor Guide, and reviewers will learn new things to look out for until they catch (almost) everything the expert would, at which points they will be experts themselves.
All repos should enable Dependabot security updates and Dependabot version updates for its own dependencies, as well as for any dependencies that are used in the repo’s CI/CD pipeline.
You can take a look at the Dependabot configuration for the SDK for an example.
Dependabot will open PRs to update dependencies and these PRs should be reviewed and merged by the team.
Some guidelines for reviewing and merging Dependabot PRs:
@dependabot rebase
on the PR.@dependabot merge
on the PR.This section describes the project permissioning model and project-level settings which are required for our merge process to work correctly.
Branch Protection Rules
settings
Per repo, these settings are found at Settings
> Branches
:
There should be a branch protection rule that applies to the main
branch. The rule should have the following settings:
meltano/engineering
)Important: The GitHub UI requires you to hit “Save changes” after making changes to a branch protection rule.
Pull requests
settings
Per repo, these settings are found at Settings
> General
> Pull Requests
: