Can’t you currently open a PR with the right commit message, have do review there with all comments posted back to JIRA, run CI on it and then merge it closing the PR? This is the basic workflow you are proposing yes?
It is the reviewer and authors job to make sure CI ran and didn’t introduce new failing tests, it doesn’t matter how they were ran. It is just as easy to let something through when “pr triggered” tests have a failure as it is tests manually linked from a JIRA comment, if the author and reviewer think the failures are not new. If someone want to setup some extra niceties, like auto triggered builds or something, to happen if people use the PR workflow, then I see no problem there. But I don’t think we need to force use of PRs. This is why I don’t think we need to “switch” to using PR’s. There is no need to switch. People can “also” use PRs. If someone who likes the PR workflow sets up some more nice stuff to happen when it is used, that would probably encourage more people to do things that way. But it doesn’t need to be forced. > On Jan 22, 2020, at 9:53 PM, David Capwell <dcapw...@gmail.com> wrote: > > Sorry Jeremiah, I don't understand your comment, would it be possible to > elaborate more? > > About the point on not forbidding as long as the review and testing needs > are met, could you define what that means to you? > > There are a few questions I ask myself > > "Does the current process stop code which breaks the build from merging?" > And > "How long does it take for regressions to get noticed" > > If I take myself as a example, I added a test which always failed in > CircleCI (I assume Jenkins as well), this got merged, and the jira to fix > it was around 3 months later. I am personally trying to find ways to > detect issues faster, but also see that test fail frequently (unit, jvm > dtest, python dtest, etc.) so it's easy for this to slip through. > > My mind set is that by switching to PRs (even if all the conversations are > in JIRA) we can setup automation which helps detect issues before merging. > >> On Wed, Jan 22, 2020, 7:00 PM J. D. Jordan <jeremiah.jor...@gmail.com> >> wrote: >> >> Doesn’t this github review workflow as described work right now? It’s >> just not the “only” way people do things? >> >> I don’t think we need to forbid other methods of contribution as long as >> the review and testing needs are met. >> >> -Jeremiah >> >>>> On Jan 22, 2020, at 6:35 PM, Yifan Cai <yc25c...@gmail.com> wrote: >>> >>> +1 nb to the PR approach for reviewing. >>> >>> >>> And thanks David for initiating the discussion. I would like to put my 2 >>> cents in it. >>> >>> >>> IMO, reviews comments are better associated with the changes, precisely >> to >>> the line level, if they are put in the PR rather than in the JIRA >> comments. >>> Discussions regarding each review comments are naturally organized into >>> this own dedicated thread. I agree that JIRA comments are more suitable >> for >>> high-level discussion regarding the design. But review comments in PR can >>> do a better job at code-level discussion. >>> >>> >>> Another benefit is to relief reviewers’ work. In the PR approach, we can >>> leverage the PR build step to perform an initial qualification. The >> actual >>> review can be deferred until the PR build passes. So reviewers are sure >>> that the change is good at certain level, i.e. it builds and the tests >> can >>> pass. Right now, contributors volunteer for providing the link to CI test >>> (however, one still needs to open the link to see the result). >>> >>>> On Wed, Jan 22, 2020 at 3:16 PM David Capwell <dcapw...@gmail.com> >> wrote: >>>> >>>> Thanks for the links Benedict! >>>> >>>> Been reading the links and see the following points being made >>>> >>>> *) enabling the spark process would lower the process to enter the >> project >>>> *) high level discussions should be in JIRA [1] >>>> *) not desirable to annotation JIRA and Github; should only annotate >> JIRA >>>> (reviewer, labels, etc.) >>>> *) given the multi branch nature, pull requires are not intuitive [2] >>>> *) merging is problematic and should keep the current merge process >>>> *) commits@ is not usable with PRs >>>> *) commits@ is better because of PRs >>>> *) people are more willing to nit-pick with PRs, less likely with >> current >>>> process [3] >>>> *) opens potential to "prevent commits that don't pass the tests" [4] >>>> *) prefer the current process >>>> https://urldefense.proofpoint.com/v2/url?u=http-3A__cassandra.apache.org_doc_latest_development_patches.html&d=DwIFaQ&c=adz96Xi0w1RHqtPMowiL2g&r=CNZK3RiJDLqhsZDG6FQGnXn8WyPRCQhp4x_uBICNC0g&m=8ytcWFMDYEPmp0dPd-upl_fBM0p1Yg59TmWwfX-wul4&s=VcidG8VBMY2hhNtOULVNpvbfSdq4tMobD6JxCLB91J8&e= >>>> [5] >>>> *) current process is annoying since you have to take the link in github >>>> and attach to JIRA for each comment in review >>>> *) missed notifications, more trust in commits@ >>>> *) if someone rewrites history, comments could be hard to see >>>> *) its better to leave comments in the source code so people don't need >> to >>>> lookup github >>>> >>>> Here is how i see some of the points >>>> >>>> 1) I agree with the point that the high level discussions should be in >>>> JIRA; PRs are better at specific review and offer no real benefit over >> JIRA >>>> for larger structural changes >>>> 2) there are different patterns with multiple branches as well, but >> some of >>>> it is possible to codify and include in CI. For example, you could take >>>> the diff, attempt to apply to 2.2 (maybe if [dtest] in commit?) and >> forward >>>> merge; of any conflicts are found, could annotate JIRA that the change >> is >>>> complex and may be best to submit multiple PRs. Assuming we want >> something >>>> like this, it is also possible to run the tests against those branches >> as >>>> well. I am not saying we do this, but saying that it is possible to >>>> improve or solve this problem, so doesn't appear a blocker to me. >>>> 3) by marking it easier to comment i can definitely see this happen, but >>>> don't see this as a reason not to. I find that you are more willing to >>>> actually talk about small sections of the code in PR than in other forms >>>> and that its easier to track. One of the things i see now is that the >>>> conversation moves to slack, so is it better not happening, happening in >>>> slack, or happening in github? >>>> 4) This is actually why i started this thread. I created a patch a >> while >>>> back that passed review, got merged, and has been failing the build ever >>>> since. I would like to make it more clear that code is likely to do >> this >>>> or not. >>>> 5) The link documents the process as submitting patches generate by "git >>>> format-patch", which i was told not to do my first patch >>>> >>>> Think i summarized all I saw. >>>> >>>>> On Wed, Jan 22, 2020 at 2:30 PM Dinesh Joshi <djo...@apache.org> >> wrote: >>>>> >>>>> I personally use Github PRs to discuss the changes if there is feedback >>>> on >>>>> the code. The discussion does get linked with the JIRA ticket. However, >>>>> committing is manual. >>>>> >>>>> Dinesh >>>>> >>>>>> On Jan 22, 2020, at 2:20 PM, David Capwell <dcapw...@gmail.com> >> wrote: >>>>>> >>>>>> When submitting or reviewing a change in JIRA I notice that we have >>>> three >>>>>> main patterns for doing this: link branch, link diff, and link GitHub >>>>> pull >>>>>> request (PR); I wanted to bring up the idea of switching over to >> GitHub >>>>>> pull requests as the norm. >>>>>> >>>>>> >>>>>> Why should we do this? The main reasons I can think of are: >>>> consistency >>>>>> within the project, common pattern outside and inside Apache (not a >> new >>>>>> process for new members to learn), >>>>>> >>>>>> PRs are easier to review and comment on (much easier than linking >> lines >>>>> in >>>>>> a branch), Github and JIRA integration is already present so all >>>>>> conversations will be added to the JIRA work log, and could be linked >>>>> with >>>>>> Jenkins to trigger builds and tests and to report the status into >> JIRA. >>>>>> >>>>>> >>>>>> How would one start to do this? >>>>>> >>>>>> >>>>>> >>>>>> 1. Include the JIRA link in a commit message (example: >>>>>> CASSANDRA-<number> : message) >>>>>> 2. Create pull request (when creating the branch, the git message >>>>>> provides a link to create a pull request) >>>>>> >>>>>> >>>>>> That is it, by doing those two steps JIRA will be updated with all >>>> Github >>>>>> conversations, Jenkins could be notified and start building and report >>>>> back >>>>>> to JIRA. >>>>>> >>>>>> >>>>>> Thoughts? >>>>>> >>>>>> >>>>>> References: >>>>>> >>>>>> - >>>>>> https://urldefense.proofpoint.com/v2/url?u=https-3A__www.apache.org_dev_svngit2jira.html&d=DwIFaQ&c=adz96Xi0w1RHqtPMowiL2g&r=CNZK3RiJDLqhsZDG6FQGnXn8WyPRCQhp4x_uBICNC0g&m=8ytcWFMDYEPmp0dPd-upl_fBM0p1Yg59TmWwfX-wul4&s=ZGHkwN50eCeHaKynhby_WK7lnXmVUSuCSYWfMtPPNxw&e= >>>>>> >>>>> >>>>> >>>>> --------------------------------------------------------------------- >>>>> To unsubscribe, e-mail: dev-unsubscr...@cassandra.apache.org >>>>> For additional commands, e-mail: dev-h...@cassandra.apache.org >>>>> >>>>> >>>> >> >> --------------------------------------------------------------------- >> To unsubscribe, e-mail: dev-unsubscr...@cassandra.apache.org >> For additional commands, e-mail: dev-h...@cassandra.apache.org >> >> --------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscr...@cassandra.apache.org For additional commands, e-mail: dev-h...@cassandra.apache.org
