Let me be a troll, but couldn't we just disable push to master for committers?
That would make smaller backports harder, but it would also eliminate unreviewed commits. And we wouldn't need to draw a line between minor for direct push and bigger changes that need review. On Wed, Feb 19, 2020 at 1:27 PM Piotr Nowojski <pi...@ververica.com> wrote: > Hi, > > I agree with question posted by Till, what problems are we trying to solve? > > Regarding merging unreviewed commits, I’m +1 for disallowing those things. > Anything that is trivial enough that I would accept as being merged without > review, is also trivial enough to review. The additional review overhead is > for me just too small compared to an extra pair of eyes that can catch > something that seemed trivial for me, but it wasn’t. > > Regarding the [minor] infix… I don’t like it as it makes commit messages > even longer (`[FLINK-12345][minor][runtime][tests]` oO?) and I didn’t > notice any big issues with reviewed `[hotifx]` commits. For one thing, it > is super rare for me to wonder which PR did this hotfix belong. And even if > I do this, just looking for some jira ticket around the notify with the > same author should be enough. But all in all, it’s not that longer commit > messages that big of a deal (just GitHub’s UI doesn’t like it). > > Piotrek > > > Side comment: the problem of not knowing where a commit came from could > be fixed by always including merge commits, just saying... ;-) > > Blasphemy > > > On 19 Feb 2020, at 12:55, Aljoscha Krettek <aljos...@apache.org> wrote: > > > > Side comment: the problem of not knowing where a commit came from could > be fixed by always including merge commits, just saying... ;-) > > > > Aljoscha > > > > On 19.02.20 11:43, Robert Metzger wrote: > >> Thank you for your response. Your response makes me realize that I > should > >> have first asked whether other committers consider the amount of hotfix > >> commits problematic or not. > >> From the number of responses to my message, I have the feeling that most > >> committers are not concerned. > >> I personally believe that the amount of hotfix commits is too high. > There > >> have been a few cases of hotfixes committed recently that have been > >> commented by other committers, have been reverted quickly after or > amended > >> in another hotfix. My hope is that a proper PR review would have caught > >> some of those cases. > >> Additionally, there's almost no papertrail around hotfixes. > >> Regular commits have a JIRA ticket, which usually has some problem > >> statement and ideally some discussion, and a pull request review. > >> Since we are using the Merge button in PRs quite frequently, we also > don't > >> have the "This closes #12345" message in the commit anymore, which > would at > >> least point to the review PR ... so when I'm checking the commit > history, I > >> need to manually search the PR history as well, to see if there is more > >> context on a commit in a PR review or not. (Hence my minor vs hotfix > >> proposal) > >> I'm happy to leave things as-is for now. As long as there are still > people > >> catching mistakes made in hotfix commits, I don't see a decline in > quality. > >> On Tue, Feb 18, 2020 at 7:58 PM Till Rohrmann <trohrm...@apache.org> > wrote: > >>> Thanks for raising this point Robert. I think it is important to > remind the > >>> community about the agreed practices once in a while. > >>> > >>> In most of the cases I had the impression that the majority of the > >>> community sticks to the agreed rules. W/o more detailed numbers (how > many > >>> of the hotfix commits are of category b) I think this discussion makes > only > >>> limited sense. Moreover, what is the underlying problem you would like > to > >>> solve here? Is it that too many commits have the hotfix tag in the > commit > >>> log? Is it that it's hard to figure out with which PR a hotfix commit > has > >>> been merged? Or did you observe a decline in Flink's quality because > of too > >>> many unreviewed changes? If we are discussing the latter case, then I > think > >>> it is very urgent. In the former cases I'm not entirely sure whether > this > >>> is an immediate problem because from what I have seen people include > many > >>> more clean up/hotfix commits in their PRs these days. > >>> > >>> Concerning the proposed practice with the [minor] tag I'm a bit torn. > The > >>> benefit of not doing it like this is that it's easier to see which > commits > >>> need to be cherry-picked if a feature needs to be ported, for example. > On > >>> the other hand if the commits are not unrelated and the feature > commits use > >>> parts of the hotfix commits, then it makes the cherry-picking more > tricky > >>> and the commits should have the JIRA issue tag. But I would be fine > with > >>> trying it out. > >>> > >>> Cheers, > >>> Till > >>> > >>> On Mon, Feb 17, 2020 at 3:56 PM Robert Metzger <rmetz...@apache.org> > >>> wrote: > >>> > >>>> Hi all, > >>>> > >>>> I would like to revive this very old thread on the topic of > "unreviewed > >>>> hotfixes on master" again. > >>>> Out of the 35 commits listed on the latest commits page on GitHub, 18 > >>> have > >>>> the tag "hotfix", on the next page it is 9, then 16, 17, ... > >>>> In the last 140 commits, 42% were hotfixes. > >>>> > >>>> For the sake of this discussion, let's distinguish between two types > of > >>>> hotfixes: > >>>> a) *reviewed hotfix commits*: They have been reviewed through a pull > >>>> request, then committed to master. > >>>> b) *unreviewed hotfix commits*: These have been pushed straight to > >>> master, > >>>> without a review. > >>>> > >>>> It's quite difficult to find out whether a hotfix has been reviewed or > >>> not > >>>> (because many hotfix commits are reviewed & pushed as part of a PR), > but > >>>> here are some recent examples of commits where I could not find > evidence > >>> of > >>>> a pull request: > >>>> > >>>> // these could probably be combined into on JIRA ticket, as they > affect > >>> the > >>>> same component + they touch dependencies > >>>> 47a1725ae14a772ba8590ee97dffd7fdf5bc04b2 [hotfix][docs][conf] Log > >>> included > >>>> packages / excluded classes > >>>> a5894677d95336a67d5539584b9204bcdd14fac5 [hotfix][docs][conf] Setup > >>> logging > >>>> for generator > >>>> 325927064542c2d018f9da33660c1cdf57e0e382 [hotfix][docs][conf] Add > query > >>>> service port to port section > >>>> 3c696a34145e838c046805b36553a50ec9bfbda0 [hotfix][docs][conf] Add > query > >>>> service port to port section > >>>> > >>>> // dependency change > >>>> 736ebc0b40abab88902ada3f564777c3ade03001 [hotfix][build] Remove > various > >>>> unused test dependencies > >>>> > >>>> // more than a regeneration / typo / compile error change > >>>> 30b5f6173e688ea20b82226db6923db19dec29a5 [hotfix][tests] Adjust > >>>> FileUtilsTest.testDeleteSymbolicLinkDirectory() to handle unsupported > >>>> situations in Windows > >>>> fc59aa4ecc2a7170bfda14ffadf0a30aa2b793bf [FLINK-16065][core] Unignore > >>>> FileUtilsTest.testDeleteDirectoryConcurrently() > >>>> > >>>> // dependency changes > >>>> fe7145787a7f36b21aad748ffea4ee8ab03c02b7 [hotfix][build] Remove unused > >>>> akka-testkit dependencies > >>>> dd34b050e8e7bd4b03ad0870a432b1631e1c0e9d [hotfix][build] Remove unused > >>>> shaded-asm7 dependencies > >>>> > >>>> // dependency changes > >>>> 244d2db78307cd7dff1c60a664046adb6fe5c405 [hotfix][web][build] Cleanup > >>>> dependencies > >>>> > >>>> In my opinion, everything that is not a typo, a compile error > (breaking > >>> the > >>>> master) or something generated (like parts of the docs) should go > >>> through a > >>>> quick pull request. > >>>> Why? I don't think many people review changes in the commit log in a > way > >>>> they review pull request changes. > >>>> > >>>> In addition to that, I propose to prefix hotfixes that have been > added as > >>>> part of a ticket with that ticket number. > >>>> So instead of "[hotfix] Harden kubernetes test", we do > >>>> "[FLINK-13978][minor] > >>>> Harden kubernetes test". > >>>> Why? For people checking the commit history, it is much easier to see > if > >>> a > >>>> hotfix has been reviewed as part of a JIRA ticket review, or whether > it > >>> is > >>>> a "hotpush" hotfix. > >>>> > >>>> For changes that are too small for a JIRA ticket, but need a review, I > >>>> propose to use the "[minor]" tag. A good example of such a change is > >>> this: > >>>> > >>>> > >>> > https://github.com/apache/flink/commit/0dc4e767c9c48ac58430a59d05185f2b071f53f5 > >>>> > >>>> My tagging minor changes accordingly in the pull requests, it is also > >>>> easier for fellow committers to quickly check them. > >>>> > >>>> Summary: > >>>> [FLINK-XXXX]: regular, reviewed change > >>>> [FLINK-XXXX][minor]: minor, unrelated changes reviewed with a regular > >>>> ticket > >>>> [minor]: minor, reviewed change > >>>> [hotfix]: unreviewed change that fixes a typo, compile error or > something > >>>> generated > >>>> > >>>> > >>>> What's your opinion on this? > >>>> > >>>> > >>>> On Sat, May 28, 2016 at 1:36 PM Vasiliki Kalavri < > >>>> vasilikikala...@gmail.com> > >>>> wrote: > >>>> > >>>>> Hi all, > >>>>> > >>>>> in principle I agree with Max. I personally avoid hotfixes and always > >>>> open > >>>>> a PR, even for javadoc improvements. > >>>>> > >>>>> I believe the main problem is that we don't have a clear definition > of > >>>> what > >>>>> constitutes a "hotfix". Ideally, even cosmetic changes and > >>> documentation > >>>>> should be reviewed; I've seen documentation added as a hotfix that > had > >>>>> spelling mistakes, which led to another hotfix... Using hotfixes to > do > >>>>> major refactoring or add features is absolutely unacceptable, in my > >>> view. > >>>>> On the other hand, with the current PR load it's not practical to ban > >>>>> hotfixes all together. > >>>>> > >>>>> I would suggest to update our contribution guidelines with some > >>>> definition > >>>>> of a hotfix. We could add a list of questions to ask before pushing > >>> one. > >>>>> e.g.: > >>>>> - does the change fix a spelling mistake in the docs? => hotfix > >>>>> - does the change add a missing javadoc? => hotfix > >>>>> - does the change improve a comment? => hotfix? > >>>>> - is the change a small refactoring in a code component you are > >>>> maintainer > >>>>> of? => hotfix > >>>>> - did you change code in a component you are not very familiar with / > >>> not > >>>>> the maintainer of? => open PR > >>>>> - is this major refactoring? (e.g. more than X lines of code) => open > >>> PR > >>>>> - does it fix a trivial bug? => open JIRA and PR > >>>>> > >>>>> and so on... > >>>>> > >>>>> What do you think? > >>>>> > >>>>> Cheers, > >>>>> -V. > >>>>> > >>>>> On 27 May 2016 at 17:40, Greg Hogan <c...@greghogan.com> wrote: > >>>>> > >>>>>> Max, > >>>>>> > >>>>>> I certainly agree that hotfixes are not ideal for large refactorings > >>>> and > >>>>>> new features. Some thoughts ... > >>>>>> > >>>>>> A hotfix should be maven verified, as should a rebased PR. Travis is > >>>>> often > >>>>>> backed up for half a day or more. > >>>>>> > >>>>>> Is our Jira and PR process sufficiently agile to handle these > >>> hotfixes? > >>>>>> Will committers simply include hotfixes with other PRs, and would it > >>> be > >>>>>> better to retain these as smaller, separate commits? > >>>>>> > >>>>>> For these cosmetic changes and small updates will the Jira and PR > >>> yield > >>>>>> beneficial documentation addition to what is provided in the commit > >>>>>> message? > >>>>>> > >>>>>> Counting hotfixes by contributor, the top of the list looks as I > >>> would > >>>>>> expect. > >>>>>> > >>>>>> Greg > >>>>>> > >>>>>> Note: this summary is rather naive and includes non-squashed hotfix > >>>>> commits > >>>>>> included in a PR > >>>>>> $ git shortlog --grep 'hotfix' -s -n release-0.9.0.. > >>>>>> 94 Stephan Ewen > >>>>>> 42 Aljoscha Krettek > >>>>>> 20 Till Rohrmann > >>>>>> 16 Robert Metzger > >>>>>> 13 Ufuk Celebi > >>>>>> 9 Fabian Hueske > >>>>>> 9 Maximilian Michels > >>>>>> 6 Greg Hogan > >>>>>> 5 Stefano Baghino > >>>>>> 3 smarthi > >>>>>> 2 Andrea Sella > >>>>>> 2 Gyula Fora > >>>>>> 2 Jun Aoki > >>>>>> 2 Sachin Goel > >>>>>> 2 mjsax > >>>>>> 2 zentol > >>>>>> 1 Alexander Alexandrov > >>>>>> 1 Gabor Gevay > >>>>>> 1 Prez Cannady > >>>>>> 1 Steve Cosenza > >>>>>> 1 Suminda Dharmasena > >>>>>> 1 chengxiang li > >>>>>> 1 jaoki > >>>>>> 1 kl0u > >>>>>> 1 qingmeng.wyh > >>>>>> 1 sksamuel > >>>>>> 1 vasia > >>>>>> > >>>>>> On Fri, May 27, 2016 at 6:10 AM, Maximilian Michels <m...@apache.org > > > >>>>>> wrote: > >>>>>> > >>>>>>> Hi Flinksters, > >>>>>>> > >>>>>>> I'd like to address an issue that has been concerning me for a > >>> while. > >>>>>>> In the Flink community we like to push "hotfixes" to the master. > >>>>>>> Hotfixes come in various shapes: From very small cosmetic changes > >>>>>>> (JavaDoc) to major refactoring and even new features. > >>>>>>> > >>>>>>> IMHO we should move away from these hotfixes. Here's why: > >>>>>>> > >>>>>>> 1) They tend to break the master because they lack test coverage > >>>>>>> 2) They are usually not communicated with the maintainer or person > >>>>>>> working on the part being fixed > >>>>>>> 3) They are not properly documented for future reference or > >>>> follow-ups > >>>>>>> (JIRA/Github) > >>>>>>> > >>>>>>> That's why I have chosen not to push hotfixes anymore. Even for > >>> small > >>>>>>> fixes, I'll open a JIRA/Github issue. The only exception might be > >>>>>>> fixing a comment. It improves communication, documentation, and > >>> test > >>>>>>> coverage. All this helps to mature Flink and develop the community > >>> in > >>>>>>> a transparent way. > >>>>>>> > >>>>>>> I'm not sure what our contribution guidelines say about this but I > >>>>>>> would like to update them to explicitly address hotfixes. Let me > >>> know > >>>>>>> what you think. > >>>>>>> > >>>>>>> Best, > >>>>>>> Max > >>>>>>> > >>>>>> > >>>>> > >>>> > >>> > >
