You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by GitBox <gi...@apache.org> on 2020/01/24 21:38:17 UTC
[GitHub] [incubator-superset] willbarrett opened a new pull request #9018:
Add revert guidelines to CONTRIBUTING.md
willbarrett opened a new pull request #9018: Add revert guidelines to CONTRIBUTING.md
URL: https://github.com/apache/incubator-superset/pull/9018
### CATEGORY
Choose one
- [ ] Bug Fix
- [ ] Enhancement (new features, refinement)
- [ ] Refactor
- [ ] Add tests
- [ ] Build / Development Environment
- [X] Documentation
### SUMMARY
<!--- Describe the change below, including rationale and design decisions -->
Adds a section to CONTRIBUTING.md that describes the responsibilities of engineers attempting to perform a revert on Superset.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
<!--- Skip this if not applicable -->
n/a
### TEST PLAN
<!--- What steps should be taken to verify the changes -->
n/a
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [ ] Has associated issue:
- [ ] Changes UI
- [ ] Requires DB Migration.
- [ ] Confirm DB Migration upgrade and downgrade tested.
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
### REVIEWERS
@mistercrunch @john-bodley @graceguo-supercat @betodealmeida @etr2460 @craig-rueda @dpgaspar
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] etr2460 commented on a change in pull request
#9018: Add revert guidelines to CONTRIBUTING.md
Posted by GitBox <gi...@apache.org>.
etr2460 commented on a change in pull request #9018: Add revert guidelines to CONTRIBUTING.md
URL: https://github.com/apache/incubator-superset/pull/9018#discussion_r370858511
##########
File path: CONTRIBUTING.md
##########
@@ -226,6 +227,21 @@ If the PR passes CI tests and does not have any `need:` labels, it is ready for
If an issue/PR has been inactive for >=30 days, it will be closed. If it does not have any status label, add `inactive`.
+## Revert Guidelines
+
+Reverting changes that are causing issues in the master branch is a normal and expected part of the development process. In an open source community, the ramifications of a change cannot always be fully understood. With that in mind, here are some considerations to keep in mind when considering a revert:
Review comment:
Do you feel like age of the PR is something to consider? I'd say recent PRs are more acceptable to revert than very old ones for a couple reasons: it means the regression was discovered quickly so it likely affecting multiple users and it's unlikely the revert will cause issues due to other logic committed after the bad PR.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] willbarrett commented on issue #9018: Add
revert guidelines to CONTRIBUTING.md
Posted by GitBox <gi...@apache.org>.
willbarrett commented on issue #9018: Add revert guidelines to CONTRIBUTING.md
URL: https://github.com/apache/incubator-superset/pull/9018#issuecomment-579397900
@graceguo-supercat we have intentionally avoided adding any hard and fast rules. This will always be a subjective decision. We have added responsibilities for the engineer reverting code changes around communication. I think this is the fundamental point where we've had disagreement in the community recently. Do you think that's sufficient or are there some hard-and-fast rules that you think we should add?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] graceguo-supercat edited a comment on issue
#9018: Add revert guidelines to CONTRIBUTING.md
Posted by GitBox <gi...@apache.org>.
graceguo-supercat edited a comment on issue #9018: Add revert guidelines to CONTRIBUTING.md
URL: https://github.com/apache/incubator-superset/pull/9018#issuecomment-578896991
Hi @willbarrett Thanks for adding this guideline.
Do you have some `hard rules` to suggest for PR reverting? For example, break existed chart/queries, reported by multiple users, can't fix it for a pretty long time, .etc.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] etr2460 commented on a change in pull request
#9018: Add revert guidelines to CONTRIBUTING.md
Posted by GitBox <gi...@apache.org>.
etr2460 commented on a change in pull request #9018: Add revert guidelines to CONTRIBUTING.md
URL: https://github.com/apache/incubator-superset/pull/9018#discussion_r370898985
##########
File path: CONTRIBUTING.md
##########
@@ -226,6 +227,22 @@ If the PR passes CI tests and does not have any `need:` labels, it is ready for
If an issue/PR has been inactive for >=30 days, it will be closed. If it does not have any status label, add `inactive`.
+## Revert Guidelines
+
+Reverting changes that are causing issues in the master branch is a normal and expected part of the development process. In an open source community, the ramifications of a change cannot always be fully understood. With that in mind, here are some considerations to keep in mind when considering a revert:
+
+- **Availability of the PR author:** If the original PR author or the engineer who merged the code is highly available and can provide a fix in a reasonable timeframe, this would counter-indicate reverting.
+- **Severity of the Issue:** How severe is the problem on master? Is it keeping the project from moving forward? Is there user impact? What percentage of users will experience a problem?
+- **Size of the change being reverted:** Reverting a single small PR is a much lower-risk proposition than reverting a massive, multi-PR change.
+- **Age of the change being reverted:** Reverting a recently-merged PR will be more acceptable than reverting an older PR. A bug discovered in an older PR is unlikely to be causing widespread serious issues.
+- **Risk inherent in Reverting:** Will the reversion break critical functionality? Is the medicine more dangerous than the disease?
Review comment:
sp nit: uncapitalize `Reverting`
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] codecov-io commented on issue #9018: Add
revert guidelines to CONTRIBUTING.md
Posted by GitBox <gi...@apache.org>.
codecov-io commented on issue #9018: Add revert guidelines to CONTRIBUTING.md
URL: https://github.com/apache/incubator-superset/pull/9018#issuecomment-578329509
# [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/9018?src=pr&el=h1) Report
> Merging [#9018](https://codecov.io/gh/apache/incubator-superset/pull/9018?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-superset/commit/f079b6dad072d6cca177cbf19da900b9e61dcc5e?src=pr&el=desc) will **not change** coverage.
> The diff coverage is `n/a`.
[](https://codecov.io/gh/apache/incubator-superset/pull/9018?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #9018 +/- ##
=======================================
Coverage 59.15% 59.15%
=======================================
Files 367 367
Lines 11686 11686
Branches 2866 2866
=======================================
Hits 6913 6913
Misses 4594 4594
Partials 179 179
```
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-superset/pull/9018?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/9018?src=pr&el=footer). Last update [f079b6d...bfc0a52](https://codecov.io/gh/apache/incubator-superset/pull/9018?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] willbarrett commented on a change in pull
request #9018: Add revert guidelines to CONTRIBUTING.md
Posted by GitBox <gi...@apache.org>.
willbarrett commented on a change in pull request #9018: Add revert guidelines to CONTRIBUTING.md
URL: https://github.com/apache/incubator-superset/pull/9018#discussion_r370869393
##########
File path: CONTRIBUTING.md
##########
@@ -226,6 +227,21 @@ If the PR passes CI tests and does not have any `need:` labels, it is ready for
If an issue/PR has been inactive for >=30 days, it will be closed. If it does not have any status label, add `inactive`.
+## Revert Guidelines
+
+Reverting changes that are causing issues in the master branch is a normal and expected part of the development process. In an open source community, the ramifications of a change cannot always be fully understood. With that in mind, here are some considerations to keep in mind when considering a revert:
Review comment:
Good point. Will add.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] etr2460 commented on a change in pull request
#9018: Add revert guidelines to CONTRIBUTING.md
Posted by GitBox <gi...@apache.org>.
etr2460 commented on a change in pull request #9018: Add revert guidelines to CONTRIBUTING.md
URL: https://github.com/apache/incubator-superset/pull/9018#discussion_r370899015
##########
File path: CONTRIBUTING.md
##########
@@ -226,6 +227,22 @@ If the PR passes CI tests and does not have any `need:` labels, it is ready for
If an issue/PR has been inactive for >=30 days, it will be closed. If it does not have any status label, add `inactive`.
+## Revert Guidelines
+
+Reverting changes that are causing issues in the master branch is a normal and expected part of the development process. In an open source community, the ramifications of a change cannot always be fully understood. With that in mind, here are some considerations to keep in mind when considering a revert:
+
+- **Availability of the PR author:** If the original PR author or the engineer who merged the code is highly available and can provide a fix in a reasonable timeframe, this would counter-indicate reverting.
+- **Severity of the Issue:** How severe is the problem on master? Is it keeping the project from moving forward? Is there user impact? What percentage of users will experience a problem?
Review comment:
sp nit: uncapitalize `Issue`
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] codecov-io edited a comment on issue #9018:
Add revert guidelines to CONTRIBUTING.md
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on issue #9018: Add revert guidelines to CONTRIBUTING.md
URL: https://github.com/apache/incubator-superset/pull/9018#issuecomment-578329509
# [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/9018?src=pr&el=h1) Report
> Merging [#9018](https://codecov.io/gh/apache/incubator-superset/pull/9018?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-superset/commit/f079b6dad072d6cca177cbf19da900b9e61dcc5e?src=pr&el=desc) will **not change** coverage.
> The diff coverage is `n/a`.
[](https://codecov.io/gh/apache/incubator-superset/pull/9018?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #9018 +/- ##
=======================================
Coverage 59.15% 59.15%
=======================================
Files 367 367
Lines 11686 11686
Branches 2866 2866
=======================================
Hits 6913 6913
Misses 4594 4594
Partials 179 179
```
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-superset/pull/9018?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/9018?src=pr&el=footer). Last update [f079b6d...944cb37](https://codecov.io/gh/apache/incubator-superset/pull/9018?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] mistercrunch commented on issue #9018: Add
revert guidelines to CONTRIBUTING.md
Posted by GitBox <gi...@apache.org>.
mistercrunch commented on issue #9018: Add revert guidelines to CONTRIBUTING.md
URL: https://github.com/apache/incubator-superset/pull/9018#issuecomment-579600923
I agree that this is multidimensional and fuzzy. It's hard to come up with universal rules here.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] craig-rueda merged pull request #9018: Add
revert guidelines to CONTRIBUTING.md
Posted by GitBox <gi...@apache.org>.
craig-rueda merged pull request #9018: Add revert guidelines to CONTRIBUTING.md
URL: https://github.com/apache/incubator-superset/pull/9018
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] graceguo-supercat commented on issue #9018:
Add revert guidelines to CONTRIBUTING.md
Posted by GitBox <gi...@apache.org>.
graceguo-supercat commented on issue #9018: Add revert guidelines to CONTRIBUTING.md
URL: https://github.com/apache/incubator-superset/pull/9018#issuecomment-578896991
Hi @willbarrett Thanks for adding this guideline.
Do you have some `hard rules` to apply PR reverting? For example, break existed chart/queries, reported by multiple users, can't fix it for a pretty long time, .etc.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] mistercrunch edited a comment on issue #9018:
Add revert guidelines to CONTRIBUTING.md
Posted by GitBox <gi...@apache.org>.
mistercrunch edited a comment on issue #9018: Add revert guidelines to CONTRIBUTING.md
URL: https://github.com/apache/incubator-superset/pull/9018#issuecomment-579600923
I agree that this is multidimensional and fuzzy. It's hard to come up with universal/absolute rules here.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org