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.

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

Update contributing docs and PR template according to our best practices #779

Merged
merged 21 commits into from
Jun 27, 2022

Conversation

rumyantseva
Copy link
Member

@rumyantseva rumyantseva commented Jun 23, 2022

Description

This PR closes #749.

Pull Request Template

The idea is to have a simple Pull Request Template for the community and a more complex one for the team.

After reviewing all the past drafts, I decided not to add all the tiny details from the individual checklist.

I'm not sure if we really need a dedicated template for the team. Please provide feedback about it.

Pull Request Draft

I documented the usage of Pull Request Drafts in PROCESS.md. It is based on our principles and current use cases.

Feel free to disagree and suggest improvements.

How to review

Feel free to rephrase sentences and suggest changes!

Readiness checklist

  • I set assignee, reviewers, labels, milestone, project and sprint.

@rumyantseva rumyantseva added the documentation Something user-visible is badly or not documented label Jun 23, 2022
@rumyantseva rumyantseva requested a review from AlekSi as a code owner June 23, 2022 08:48
@rumyantseva rumyantseva self-assigned this Jun 23, 2022
@codecov
Copy link

codecov bot commented Jun 23, 2022

Codecov Report

Merging #779 (07074e7) into main (96e248d) will decrease coverage by 33.86%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff             @@
##             main     #779       +/-   ##
===========================================
- Coverage   59.19%   25.33%   -33.87%     
===========================================
  Files         212      212               
  Lines        9348     9348               
===========================================
- Hits         5534     2368     -3166     
- Misses       3169     6617     +3448     
+ Partials      645      363      -282     
Impacted Files Coverage Δ
internal/handlers/pg/msg_hostinfo.go 0.00% <0.00%> (-100.00%) ⬇️
internal/handlers/pg/msg_buildinfo.go 0.00% <0.00%> (-100.00%) ⬇️
internal/handlers/pg/msg_listcommands.go 0.00% <0.00%> (-100.00%) ⬇️
internal/handlers/pg/pgdb/placeholder.go 0.00% <0.00%> (-100.00%) ⬇️
internal/handlers/pg/msg_connectionstatus.go 0.00% <0.00%> (-100.00%) ⬇️
internal/handlers/pg/msg_setfreemonitoring.go 0.00% <0.00%> (-100.00%) ⬇️
...nternal/handlers/pg/msg_getfreemonitoringstatus.go 0.00% <0.00%> (-100.00%) ⬇️
internal/handlers/common/params.go 0.00% <0.00%> (-92.73%) ⬇️
internal/util/ctxutil/ctxutil.go 0.00% <0.00%> (-88.24%) ⬇️
internal/handlers/common/update.go 0.00% <0.00%> (-87.50%) ⬇️
... and 56 more
Flag Coverage Δ
FerretDB ?
MongoDB 7.22% <ø> (ø)
integration 7.22% <ø> (-55.53%) ⬇️
unit 22.99% <ø> (+0.04%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

.github/PROCESS.md Show resolved Hide resolved
.github/PROCESS.md Outdated Show resolved Hide resolved
Do not leave original commit messages in the squashed commit body.

## Draft pull requests

1. We aim our issues to be scoped and well-described.
Copy link
Contributor

Choose a reason for hiding this comment

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

Would you please rewrite in active voice?

Copy link
Member Author

Choose a reason for hiding this comment

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

Can you suggest the exact sentence? Issues are not actors, so it's hard to come up with active voice for them :) We aim is written in active voice.

Copy link
Contributor

Choose a reason for hiding this comment

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

Please see the comment #779 (comment).
I challenge the entire paragraph for its sense, existence, and meaning.

.github/PROCESS.md Show resolved Hide resolved
With that, ideally, there shouldn't be a need to create a draft pull request.
2. We aim to implement and merge improvements as fast as possible to continuously increase
[Lead Time for Changes](https://cloud.google.com/blog/products/devops-sre/using-the-four-keys-to-measure-your-devops-performance).
Having too many drafts indicates that something is wrong in our processes, it needs to be discussed and changed.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Having too many drafts indicates that something is wrong in our processes, it needs to be discussed and changed.
Too many drafts indicate that something is wrong in our processes. Then we need to discuss and change it.

.github/PULL_REQUEST_TEMPLATE.md Show resolved Hide resolved
.github/PULL_REQUEST_TEMPLATE.md Show resolved Hide resolved
* [ ] I set assignee, reviewers, labels, milestone, project and sprint.
* [ ] I added tests for new functionality or bugfixes.
* [ ] I ran `task all`, and it passed.
* [ ] I documented exported and unexported functions, variables, types, etc.
Copy link
Contributor

Choose a reason for hiding this comment

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

And I changed the documentation in changed functions correspondingly.

.github/PROCESS.md Outdated Show resolved Hide resolved
@rumyantseva rumyantseva changed the title Update contributing docs and PR template according to our best practices Update contributing docs and PR template according to our best practices test :) Jun 23, 2022
@rumyantseva rumyantseva changed the title Update contributing docs and PR template according to our best practices test :) Update contributing docs and PR template according to our best practices Jun 23, 2022
.github/PROCESS.md Outdated Show resolved Hide resolved
.github/PROCESS.md Outdated Show resolved Hide resolved
.github/PROCESS.md Outdated Show resolved Hide resolved
.github/PULL_REQUEST_TEMPLATE.md Show resolved Hide resolved
@AlekSi
Copy link
Member

AlekSi commented Jun 27, 2022

Merging as it is now because we need to move forward with it. We will change those files over time.

@AlekSi AlekSi merged commit ddaa0d1 into FerretDB:main Jun 27, 2022
@rumyantseva rumyantseva deleted the issue-749-contributing-ugidelines branch August 8, 2022 10:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Something user-visible is badly or not documented
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update contributing docs and PR template / checklist
4 participants