-
Notifications
You must be signed in to change notification settings - Fork 749
docs: Define how to handle ADR PRs #13644
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
docs: Define how to handle ADR PRs #13644
Conversation
Code Climate has analyzed commit e227437 and detected 0 issues on this pull request. The test coverage on the diff in this pull request is 100.0% (50% is the threshold). This pull request will bring the total coverage in the repository to 61.9% (0.0% change). View more on Code Climate. |
@@ -86,7 +86,9 @@ All issues and pull requests for product removal must use the [product-removal]( | |||
#### Architectural Decision Records | |||
- We use [Architectural Decision Records](#introduction-of-architecture-decisions-records) | |||
- The first ADR defines the context and guidelines for new ADRs. | |||
|
|||
- All PR that support or justify a proposed ADR should be linked to that ADR in the description and should remain in the draft state until ADR is accepted. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I imagine cases where a new contributor opens a PR without knowing it could be impacted or related to a proposed ADR. But maintainers would be aware of this during the review. So, we could also include a sentence on how maintainers should proceed in this case. Basically to inform other reviewers and also guide the author through the process.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using dedicated label could be good to easily bring visibility on these PRs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added something along the lines.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. Just a few minor corrections if you want to take them.
@@ -86,7 +86,9 @@ All issues and pull requests for product removal must use the [product-removal]( | |||
#### Architectural Decision Records | |||
- We use [Architectural Decision Records](#introduction-of-architecture-decisions-records) | |||
- The first ADR defines the context and guidelines for new ADRs. | |||
|
|||
- All PR that support or justify a proposed ADR should be linked to that ADR in the description and should remain in the draft state until ADR is accepted. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- All PR that support or justify a proposed ADR should be linked to that ADR in the description and should remain in the draft state until ADR is accepted. | |
- All PRs that support or justify a proposed ADR should be linked to that ADR in the description and should remain in the draft state until ADR is accepted. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed, thanks.
@@ -86,7 +86,9 @@ All issues and pull requests for product removal must use the [product-removal]( | |||
#### Architectural Decision Records | |||
- We use [Architectural Decision Records](#introduction-of-architecture-decisions-records) | |||
- The first ADR defines the context and guidelines for new ADRs. | |||
|
|||
- All PR that support or justify a proposed ADR should be linked to that ADR in the description and should remain in the draft state until ADR is accepted. | |||
In case a PR linked to an ADR would be found valuable to the project in isolation from the ADR, reviewers may decide to unlink the PR from ADR, switch it |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In case a PR linked to an ADR would be found valuable to the project in isolation from the ADR, reviewers may decide to unlink the PR from ADR, switch it | |
In case a PR linked to an ADR is found to be valuable to the project in isolation from the ADR, reviewers may decide to unlink the PR from ADR, switch it |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed, thanks.
|
||
- All PR that support or justify a proposed ADR should be linked to that ADR in the description and should remain in the draft state until ADR is accepted. | ||
In case a PR linked to an ADR would be found valuable to the project in isolation from the ADR, reviewers may decide to unlink the PR from ADR, switch it | ||
to the ready state and proceed with the PR in a regular way. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
to the ready state and proceed with the PR in a regular way. | |
to the ready state and proceed with the PR via the normal process. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed, thanks.
I'll add the |
I'm not seeing any new commits. Did you forget to push? |
e227437
to
5772d3b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm, thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The content LGTM. There is just a minor typo.
We should keep the changes related to a proposed ADR at bay until the ADR is accepted. It'd allow us to protect the project from proposal creep-in.
5772d3b
to
08b3b7c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thanks
We have three approvals. Merging, thanks! |
Description:
Rationale:
Review Hints:
Alternative for #13614.