Introduce our code culture in contributing.md #1096
Conversation
|
Looks good! Thank you! |
CONTRIBUTING.md
Outdated
| - Every user-facing feature that is NOT behind a feature gate should have an integration test | ||
|
|
||
| ### Pull requests | ||
| - One core-team member has to approve the PR to be able to merge (defined in code owners file) |
There was a problem hiding this comment.
| - One core-team member has to approve the PR to be able to merge (defined in code owners file) | |
| - One code owner has to approve the PR to be able to merge. |
There was a problem hiding this comment.
What is "an owner"? The OWNERS file only has approvers and reviewers.
There was a problem hiding this comment.
Let's delete OWNERS until when/if we go back to Prow. CODEOWNERS only has the concept of an owner.
There was a problem hiding this comment.
I think they are still valuable because github assigns PRs based on them. I still feel like they are a meaningful definition of what we consider a "core-team member"
There was a problem hiding this comment.
I've rephrased it a bit @porridge let me know if that works for you
CONTRIBUTING.md
Outdated
| - Every user-facing feature that is NOT behind a feature gate should have an integration test | ||
|
|
||
| ### Pull requests | ||
| - One core-team member has to approve the PR to be able to merge (defined in code owners file) |
There was a problem hiding this comment.
What is "an owner"? The OWNERS file only has approvers and reviewers.
CONTRIBUTING.md
Outdated
| ### Pull requests | ||
| - One core-team member has to approve the PR to be able to merge (defined in code owners file) | ||
| - When core-team member puts 'request for changes' on your PR, that stops merging until it's resolved (even when have enough approvals) | ||
| - Since KUDO is developed in multiple timezones, try to keep the PR open for everyone to be able to see it (~24h) |
There was a problem hiding this comment.
Should we mention public holidays?
There was a problem hiding this comment.
Do we need to be explicit? I like guidelines/light rules :) we're fairly senior team right now, I would be ok with letting everyone interpret what does it mean for the other timezone to see their code :) but I can surely add it if you feel like it helps
There was a problem hiding this comment.
Agreed, I'm 👍 on not needing to be explicit here...I think the intent of the culture is giving opportunities to others, and as long as we carry that intent, we can all be roughly on the same page. Just something to keep in mind before you hit that squash and merge button - have there been opportunities for my fellow KUDO developers to review this? If I'm merging without, am I justifying why this needs to be in without giving that opportunity (say, a bug fix before people in US times have had a chance to see it), etc. Nothing hard and fast.
There was a problem hiding this comment.
I don't mean enumerating holidays, I'm also all for loose guidelines.
I thought about this as a helpful hint, because personally I often forget that people on the other side of the globe might be away for a while.
The mention of just 24h here would further cement my conviction that passage of time is equivalent to providing such opportunities for review.
Co-Authored-By: Marcin Owsiany <marcin@owsiany.pl>
Co-Authored-By: Marcin Owsiany <marcin@owsiany.pl>
Co-Authored-By: Aleksey Dukhovniy <adukhovniy@mesosphere.io>
Co-Authored-By: Aleksey Dukhovniy <adukhovniy@mesosphere.io>
Co-Authored-By: Aleksey Dukhovniy <adukhovniy@mesosphere.io>
Co-Authored-By: Aleksey Dukhovniy <adukhovniy@mesosphere.io>
Co-Authored-By: Matthias Eichstedt <matthias.eichstedt@mesosphere.io>
What this PR does / why we need it:
This is a first outline of our code culture that mostly came out of retrospectives we had in the last 2 months.
This is for sure not a definitive list, let's add more points in next PRs though. Let's make this PR just about the points mentioned and if everyone is OK with them and with the wording.
Please let's keep this PR open longer so that everyone have chance to see it.