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.

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

Expanded PR section of code standards doc #6844

Merged
merged 1 commit into from
Oct 9, 2023
Merged

Expanded PR section of code standards doc #6844

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
18 changes: 18 additions & 0 deletions site/content/docs/main/code-standards.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,24 @@ toc: "true"

When opening a pull request, please fill out the checklist supplied the template. This will help others properly categorize and review your pull request.

### PR title

Make sure that the pull request title summarizes the change made (and not just "fixes issue #xxxx"):

Example PR titles:

- "Check for nil when validating foo"
- "Issue #1234: Check for nil when validating foo"
Copy link
Contributor

Choose a reason for hiding this comment

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

since we have issue placeholder in PR template, I would discourage issue ref in PR title (unless this helps with any automation, like what we have for JIRA in OADP)

But I would also add a section explaining how to use the PR template, I saw a lot of misusage

maybe update it with comments to help users fill it (example https://raw.githubusercontent.com/dora-metrics/pelorus/master/.github/PULL_REQUEST_TEMPLATE/general_template.md)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

While I don't want to require use of the issue in the title/commit message, I can see some value in including it, since PR template won't show up when looking at commit history. That's why I gave both options -- i.e. if you want the issue in title, that's fine, as long as there's also a description, but just a description is fine too.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The most important point (and the main reason for this change) is to make sure that every PR gets a descriptive title -- that "fix issue #foo" alone is insufficient.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@sseago unrelated to this PR going in, we can perhaps create github actions to ensure proper naming of PRs?


### Cherry-pick PRs

When a PR to main needs to be cherry-picked to a release branch, please wait until the main PR is merged first before creating the CP PR. If the CP PR is made before the main PR is merged, there is a risk that PR modifications in response to review comments will not make it into the CP PR.

Copy link
Member

Choose a reason for hiding this comment

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

Should add some guideline regarding title for cherry-pick PRs.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we perhaps have some automation in form of github actions which can do the manual work of Cherry Pick while also conforming to the title guidelines?

Copy link
Member

@kaovilai kaovilai Sep 26, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Like kubernetes prow /cherry-pick? would be cool.

Copy link
Member

Choose a reason for hiding this comment

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

https://prow.k8s.io/command-help#:~:text=/cherrypick%20%5Bbranch%5D

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@anshulahuja98 @kaovilai Having the ability to cherry-pick via bot would add some options. Note that this sort of automated cherry-pick might require an additional github action (or manual intervention) if there is a changelog file, since the CP PR will have a new number, so the changelog file will need to be renamed. I think that sort of automation is probably out of scope for this PR though and should be considered as a follow-on action/investigation.

Copy link
Member

Choose a reason for hiding this comment

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

@sseago reopen #2663 to continue work on adding prow GH Actions.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@kaovilai well changelog filename check but also actually changing the filename, since cherry-picking the commit will have the wrong filename. When doing this manually, after cherry-picking the commit and creating the new PR you have to go in and change the filename to match the new PR number and amend the commit.

Copy link
Member

Choose a reason for hiding this comment

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

changing the filename

github actions can do that automatically :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It will need to do it after creating the PR, though, since we don't know what the filename will be until the PR exists.

Copy link
Member

Choose a reason for hiding this comment

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

on: pr-comment
if: /cherry-pick <branch>
    cp -R . /tmp/cherry-pick
    change-file-name-to-next-pr|issue-number
    git commit --amend --no-edit --sign-off
    gh pr create --base <branch>

The Cherry-pick PR title should reference the branch it's cherry-picked to and the fact that it's a CP of a commit to main:

- "[release-1.13 CP] Issue #1234: Check for nil when validating foo"


## Adding a changelog

Authors are expected to include a changelog file with their pull requests. The changelog file
Expand Down
Loading