+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 > http://cassandra.apache.org/doc/latest/development/patches.html [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://www.apache.org/dev/svngit2jira.html > > > > > > --------------------------------------------------------------------- > > To unsubscribe, e-mail: dev-unsubscr...@cassandra.apache.org > > For additional commands, e-mail: dev-h...@cassandra.apache.org > > > > >
