Hi all, This is not a blanket restriction. My suggestion, like last time, lets try it out and this time we don't even need Apache Infra to step in. Feels like it has been requested in multiple INFRA JIRAs by so many Apache projects to enable only the squash merge button and disable the others, that this setting has been moved to a simple entry in .asf.yml file in root git repository
Again this is a setting only for the develop branch. And can be reversed with a simple commit. I agree too much policing is a bad thing, but I want to look at it as a safeguard. I have spoken to multiple developers who have mentioned they clicked the wrong button in a hurry or the page refreshed and the commit got merged in a wrong way. Regards Naba Reference : https://cwiki.apache.org/confluence/display/INFRA/.asf.yaml+features+for+git+repositories Merge buttons Projects can enable/disable the "merge PR" button in the GitHub UI and configure which actions should be allowed by adding the following configuration (or derivatives thereof): github: enabled_merge_buttons: # enable squash button: squash: true # enable merge button: merge: true # disable rebase button: rebase: false On Thu, Dec 19, 2019 at 4:54 PM Owen Nichols <onich...@pivotal.io> wrote: > This proposal in no way prevents you from getting work done. Squash is > always enabled and always the most acceptable way to bring changes to > develop. > > Please start a separate thread if you would like to revisit the community > decision to require passing PR checks. > > > On Dec 19, 2019, at 4:49 PM, Jacob Barrett <jbarr...@pivotal.io> wrote: > > > > I’m in agreement with Dan. Changes to the infrastructure to flat out > prevent things that should be self policing is annoying. This PR review > lock we have had already cost us valuable time waiting for PR pipelines to > pass that have no relevance to the commit, like CI work: I’d hat to see yet > another process enforced that Kees us from getting work done when necessary. > > > > -Jake > > > > > >> On Dec 19, 2019, at 4:43 PM, Dan Smith <dsm...@pivotal.io> wrote: > >> > >> -1 to (1) and (2). > >> > >> I think merge commits are appropriate in certain circumstances, so I > don't > >> think we should make a blanket restriction. In fact I think our release > >> process involves some merges. > >> > >> I think setting standards on what is reasonable to be an individual > commit > >> will do a lot more to clean up our history than blocking merges. We'd > >> rather not see commits like "Spotless Apply" in the history, but if > >> reasonably separate and well written commits come in as part of a > merge, I > >> think that's fine. > >> > >> -Dan > >> > >>> On Thu, Dec 19, 2019 at 4:27 PM Jinmei Liao <jil...@pivotal.io> wrote: > >>> > >>> +1 > >>> > >>>> On Thu, Dec 19, 2019, 4:05 PM Owen Nichols <onich...@pivotal.io> > wrote: > >>>> > >>>> I’d like to advance this topic from an informal request/discussion to > a > >>>> discussion of a concrete proposal. > >>>> > >>>> To recap, it sounds like there is general agreement that commit > history > >>> on > >>>> develop should be linear (no merge commits), and that the biggest > >>> obstacle > >>>> to this is that the PR merge button in GitHub creates a merge commit > by > >>>> default. > >>>> > >>>> I propose the following changes: > >>>> 1) Change GitHub settings to remove the ability to create a merge > commit. > >>>> This will make Squash & Merge the default. > >>>> > >>>> 2) Change GitHub settings to require linear history on develop. This > >>>> prevents merge commits via command-line (not recommended, but wiki > still > >>>> has instructions for merging PRs this way). > >>>> > >>>> 3) Update the PR template to change the text "Is your initial > >>> contribution > >>>> a single, squashed commit” to “Is your initial contribution squashed > for > >>>> ease of review (e.g. a single commit, or a ‘refactoring’ commit plus a > >>>> ‘fix’ commit)" > >>>> > >>>> For clarity, I am proposing no-change in the following areas: > >>>> i) Leave Rebase & Merge as an option for PRs that have been > structured to > >>>> benefit from it (this can make it easier in a bisect to see whether > the > >>>> refactoring or the “fix” broke something). > >>>> ii) Leave existing wording in the wiki as-is [stating that squashing > is > >>>> preferred]. > >>>> > >>>> > >>>> Please comment via this email thread. > >>>> -Owen > >>>> > >>>> > >>>> > >>>>> On Dec 16, 2019, at 10:49 AM, Kirk Lund <kl...@apache.org> wrote: > >>>>> > >>>>> I think it's already resolved Udo ;) > >>>>> > >>>>> Here's the problem, if I fixup a dunit test by removing all uses of > >>>> "this." > >>>>> and I rename the dunit test, then git doesn't remember that the file > >>> is a > >>>>> rename -- it forever afterwards interprets it as a new file that I > >>>> created > >>>>> if I touch more than 50% of the lines (which "this." can easily do). > If > >>>> we > >>>>> squash two commits: the rename and the cleanup of that dunit test -- > >>> then > >>>>> we effectively lose the history of that file and it shows that I > >>> created > >>>> a > >>>>> new file. > >>>>> > >>>>> Also for the record, I've been working on Geode since the beginning > >>> and I > >>>>> was never made aware of this change in our process. I never voted on > >>> it. > >>>>> I'm not a big fan of changing various details in our process every > >>> single > >>>>> week. It's very easy to miss these discussions unless someone points > it > >>>> out > >>>>> to me. > >>>>> > >>>>> On Mon, Dec 16, 2019 at 10:34 AM Udo Kohlmeyer < > ukohlme...@pivotal.io> > >>>>> wrote: > >>>>> > >>>>>> I'm not sure what this discussion is about... WE, as a community, > have > >>>>>> agreed in common practices, in two place no less... > >>>>>> > >>>>>> 1) Quoting our PR template > >>>>>> > >>>>>> > >>>>>> For all changes: > >>>>>> > >>>>>> * > >>>>>> > >>>>>> Is there a JIRA ticket associated with this PR? Is it referenced in > >>>>>> the commit message? > >>>>>> > >>>>>> * > >>>>>> > >>>>>> Has your PR been rebased against the latest commit within the > >>> target > >>>>>> branch (typically|develop|)? > >>>>>> > >>>>>> * > >>>>>> > >>>>>> ***Is your initial contribution a single, squashed commit?* > >>>>>> > >>>>>> * > >>>>>> > >>>>>> Does|gradlew build|run cleanly? > >>>>>> > >>>>>> * > >>>>>> > >>>>>> Have you written or updated unit tests to verify your changes? > >>>>>> > >>>>>> * > >>>>>> > >>>>>> If adding new dependencies to the code, are these dependencies > >>>>>> licensed in a way that is compatible for inclusion underASF 2.0 > >>>>>> <http://www.apache.org/legal/resolved.html#category-a>? > >>>>>> > >>>>>> On our PR template we call out that the initial PR commit should be > >>>>>> squashed. > >>>>>> > >>>>>> 2) > >>> https://cwiki.apache.org/confluence/display/GEODE/Code+contributions > >>>>>> < > https://cwiki.apache.org/confluence/display/GEODE/Code+contributions > >>>> > >>>>>> -- See "Accepting a PR Using the Command Line" - Point #3. > >>>>>> > >>>>>> > >>>>>> @Kirk, if each of your commits "stands alone", I commend you on the > >>>>>> diligence, but in reality, they should either then be stand alone > PR's > >>>>>> or just extra work created for yourself. > >>>>>> > >>>>>> If we want to change the way we have agreed upon we > >>> submit/commit/merge > >>>>>> changes back into develop... Then this is another discussion thread, > >>>>>> until then, I think we should all remind ourselves on our agreed > >>>>>> contributions code of conduct. > >>>>>> > >>>>>> --Udo > >>>>>> > >>>>>> On 12/16/19 9:59 AM, Nabarun Nag wrote: > >>>>>>> Kirk, I believe that creating a Pull Request with multiple commits > is > >>>> ok. > >>>>>>> It's just in the end that when it's being pushed to develop branch, > >>> it > >>>>>>> needs to be squash merged. I believe that is what you have > mentioned > >>> in > >>>>>> the > >>>>>>> first paragraph, and I am more than happy with that. > >>>>>>> If you can see in the first screenshot comparison that I had > attached > >>>> in > >>>>>>> the first email in this thread is what I want to avoid. > >>>>>>> > >>>>>>> Thank you for your feedback. > >>>>>>> > >>>>>>> Regards > >>>>>>> Naba > >>>>>>> > >>>>>>> > >>>>>>> On Mon, Dec 16, 2019 at 9:47 AM Kirk Lund <kl...@apache.org> > wrote: > >>>>>>> > >>>>>>>> Whenever I submit a PR with multiple commits that I intend to > rebase > >>>> to > >>>>>>>> develop, I always try to ensure that each commit stands alone as > is > >>>>>>>> (compiles and passes tests). Separating file renames and > refactoring > >>>>>> from > >>>>>>>> behavior changes into different commits seems very valuable to me, > >>> and > >>>>>> I've > >>>>>>>> had trouble getting people to review PRs without this separation > >>> (but > >>>> it > >>>>>>>> could be squashed as it's merged to develop). > >>>>>>>> > >>>>>>>> It sounds to me like the real problem is (a) some PRs have > multiple > >>>>>> commits > >>>>>>>> that don't compile or don't pass tests, and (b) some PRs that > should > >>>> be > >>>>>>>> merged with squash are not (by accident most likely). > >>>>>>>> > >>>>>>>> I can submit multiple PRs instead of one PR with multiple commits. > >>> So > >>>>>> I'll > >>>>>>>> change my response to -0 if that helps prevent commits to develop > >>> that > >>>>>>>> don't compile or pass tests. Without preventing rebase or merge > >>>> commits > >>>>>>>> from github, I'm not sure how we can really enforce this or > prevent > >>>> (b) > >>>>>>>> above. > >>>>>>>> > >>>>>>>> On Fri, Dec 13, 2019 at 3:38 PM Alexander Murmann < > >>>> amurm...@pivotal.io> > >>>>>>>> wrote: > >>>>>>>> > >>>>>>>>> I wonder if Kirk's and Naba's statements are necessarily at odds. > >>>>>>>>> > >>>>>>>>> Make the change easy (warning: this may be hard), then make the > >>> easy > >>>>>>>>>> change." > >>>>>>>>> -Kent Beck > >>>>>>>>> > >>>>>>>>> Following Kent Beck's advise might reasonably split into two > >>> commits. > >>>>>> One > >>>>>>>>> refactor commit and a separate commit that introduces the actual > >>>>>> change. > >>>>>>>>> They should be individually revertible and might be easier > >>> understood > >>>>>> if > >>>>>>>>> split out. I vividly remember a change on our code base where > >>> someone > >>>>>>>> did a > >>>>>>>>> huge amount of refactoring that resulted than in one parameter > >>>> changing > >>>>>>>> in > >>>>>>>>> order to get the desired functionality change. If that was in one > >>>>>> commit, > >>>>>>>>> it would be hard to see the actual change. If split out, it's > >>>> beautiful > >>>>>>>> and > >>>>>>>>> crystal clear. > >>>>>>>>> > >>>>>>>>> I am unsure how that would be reflected in terms of JIRA ticket > >>>>>>>> references. > >>>>>>>>> Usually we assume that if there is a commit with the ticket > number, > >>>> the > >>>>>>>>> issue is resolved. Maybe the key here is to create a separate > >>>>>> refactoring > >>>>>>>>> ticket. > >>>>>>>>> > >>>>>>>>> Would that allow us to have our cake and eat it too? > >>>>>>>>> > >>>>>>>>> On Fri, Dec 13, 2019 at 3:16 PM Nabarun Nag <n...@pivotal.io> > >>> wrote: > >>>>>>>>> > >>>>>>>>>> It is to help with bisect operations when things start failing > ... > >>>>>>>> helps > >>>>>>>>> us > >>>>>>>>>> it revert and build faster. > >>>>>>>>>> also with cherry picking features / fixes to previous versions . > >>>>>>>>>> And keeping the git history clean with no unnecessary “merge > >>>> commits”. > >>>>>>>>>> > >>>>>>>>>> Regards > >>>>>>>>>> Naba > >>>>>>>>>> > >>>>>>>>>> > >>>>>>>>>> On Fri, Dec 13, 2019 at 2:25 PM Kirk Lund <kl...@apache.org> > >>> wrote: > >>>>>>>>>> > >>>>>>>>>>> -1 I really like to sometimes have more than 1 commit in a PR > and > >>>>>>>> keep > >>>>>>>>>> them > >>>>>>>>>>> separate when they merge to develop > >>>>>>>>>>> > >>>>>>>>>>> On Tue, Oct 22, 2019 at 5:12 PM Nabarun Nag <n...@pivotal.io> > >>>> wrote: > >>>>>>>>>>> > >>>>>>>>>>>> Hi Geode Committers, > >>>>>>>>>>>> > >>>>>>>>>>>> A kind request for using squash commit instead of using merge. > >>>>>>>>>>>> This will really help us in our bisect operations when a > >>>>>>>>>>>> regression/flakiness in the product is introduced. We can > >>> automate > >>>>>>>>> and > >>>>>>>>>> go > >>>>>>>>>>>> through fewer commits faster, avoiding commits like "spotless > >>> fix" > >>>>>>>>> and > >>>>>>>>>>>> "re-trigger precheck-in" or other minor commits in the merged > >>>>>>>> branch. > >>>>>>>>>>>> Also, please use the commit format : (helps us to know who > >>> worked > >>>>>>>> on > >>>>>>>>>> it, > >>>>>>>>>>>> what is the history) > >>>>>>>>>>>> > >>>>>>>>>>>> > >>>>>>>>>>>> > >>>>>>>>>>>> * GEODE-xxxx: <brief intro > > >>>>>>>>>>>> * explanation line 1 * > >>> explanation > >>>>>>>>> line > >>>>>>>>>> 2* > >>>>>>>>>>>> This is not a rule or anything, but a request to help out your > >>>>>>>> fellow > >>>>>>>>>>>> committers in quickly detecting a problem. > >>>>>>>>>>>> > >>>>>>>>>>>> For inspiration, we can look into Apache Kafka / Spark where > >>> they > >>>>>>>>> have > >>>>>>>>>> a > >>>>>>>>>>>> complete linear graph for their main branch HEAD [see > >>> attachment] > >>>>>>>>>>>> > >>>>>>>>>>>> Regards > >>>>>>>>>>>> Naba. > >>>>>>>>>>>> > >>>>>>>>>>>> > >>>>>>>>>>>> > >>>>>> > >>>> > >>>> > >>> > >
