I do like the idea of adding the 'squash and merge` option so we end up with 2 Github options:
- rebase and merge - squash and merge Leave it up to the merger which option to choose. Now as far as how to create a "change log", back to what Leif said earlier: "Why not generate the ChangeLog from the actual PR “subject” line? Then it wouldn’t matter if you have 1 or 100 commits for a particular PR. The Sorber wrote a little script for ATS that generates our ChangeLogs *)" On Wed, Oct 24, 2018 at 10:24 AM Rawlin Peters <[email protected]> wrote: > I agree with most of this and wish everyone would structure their PRs > and commits like this, but the fact of the matter is that small > checkpoint commits are simply just the way some people like to work. > For the last few things I've worked on, I've tried to separate changes > to the different components for the same logical feature into > different PRs (API, UI, Traffic Router). I find that breaking down the > feature into a PR per component makes things a bit easier to review, > since not everyone has experience all the various TC components that > could be mashed into one mega-PR. If a mega-PR contains changes to TO, > TP, and TR all in one, some people maybe be comfortable reviewing TO > but not necessarily TP or TR, so you end up having to coordinate with > multiple reviewers/committers who are comfortable reviewing different > areas just to get a single PR merged. > > Unless we start making good commit messages and "one standalone > change" per commit a hard requirement for all contributors to the > project, I doubt we can ever get to this ideal, and I'm not sure we > should make that a requirement right now. However, by squashing and > merging commits, we can clean up the git history by removing all of > the non-atomic/checkpoint commits as well as other benefits that have > already been discussed. > > Nonetheless, we will still be able to use the "rebase and merge" > option at the committer's discretion. If the contributor and committer > feel that the commits in the PR are more valuable in the history > because they're atomic and each have good commit messages, the > committer should choose the "rebase and merge" option. If the PR > contains lots of small, nonatomic, checkpoint commits that would > obfuscate the git history and make it harder for someone doing a `git > blame` to figure out why a change was made, then maybe it would be > better for the committer to choose the "squash and merge" option or > kindly ask the PR submitter to squash the commits themselves into a > single commit and write a good title+message. > > - Rawlin > > On Tue, Oct 23, 2018 at 4:49 PM Chris Lemmons <[email protected]> wrote: > > > > I don't think I'm quite -1 on this, but I think we can do better. > > > > I think a PR should be a single unit of work and a commit should be a > > single unit of code. > > > > I like using atomic commits. That is, each commit does one thing. So > > if I need to revert a fix or track down when something was introduced, > > I can find it. For commit messages, I really like the seven rules > > documented here, but especially the last one: > > https://chris.beams.io/posts/git-commit/ > > > > > Use the body to explain what and why vs. how > > > > I think it's important to put the body in the commit message in order > > to explain why the change is necessary what the non-obvious effects of > > it are. Having a good subject for the commit is also very useful, > > since that's what GitHub (and really all the tools) shows when you > > scroll through commits, looking for what changed. > > > > In order to have atomic commits, sometimes you'll need one or more > > commits as part of a single PR. Not most of the time, but sometimes. > > Especially with a large feature, you may make multiple dependent > > changes, but each one is a logical unit of work. For example, if the > > task is "add support for foobaz plugin", there might be three commits, > > with summaries that look like "Add support for foobaz plugin to config > > generation", "Add UI for editing foobaz plugin config", and "Add API > > endpoints for foobaz plugin config". That also makes it easy to direct > > domain experts to each of the changesets for focused review. > > > > A PR on the other hand, is a single unit of work. While each commit > > should be stand-alone, capable of running correctly before and after, > > each commit might not be desirable on a standalone basis. For example, > > while "Add support for foobaz plugin to config generation" is a single > > unit of code in that it operates correctly, has tests, is documented, > > and is completely implemented, it's not exceptionally useful without a > > UI to edit it. It makes sense to group all these changes and merge > > them as a unit. > > > > For almost all bug fixes and most medium to small features, each PR > > will have precisely one commit. But especially for larger "projects", > > it makes the most sense to group it all as one PR, but merge it as a > > feature. > > > > Relatedly, it's very rarely correct to merge commits from two > > different authors. (This is important for legal reasons as well, if it > > ever came to it; authors license their code when they contribute it, > > so it's important to know whence it came.) However, in the event that > > two people jointly produce what amounts to a single unit of code (for > > example, by pair programming, or by collaborating on a snippet of code > > on Slack or IRC), you can document that in git by adding it to the end > > of the commit message body: > > > > > Co-authored-by: name <[email protected]> > > > Co-authored-by: another-name <[email protected]>" > > > > Details for that technique here: > > > https://help.github.com/articles/creating-a-commit-with-multiple-authors/ > > > > It's not quite a "standard", but it's quickly being adopted by most of > > the tools I use, at least. > > > > As for the "Squash" option, I actually prefer the "Rebase" choice. > > It's the same thing if you have single-commit PRs, but it handles > > multi-commit PRs a bit more nicely. > > > > So, to summarize, here's what I think: > > > > - Each commit should be an atomic unit of code and be described by a > > descriptive commit subject and body. > > - Each PR should be a logical grouping of closely related work; > > usually a single commit. > > - We should use Rebase for flattening our changes. > > - Squash is still an "okay" heuristic that accomplishes the same > > thing, most of the time. > > On Tue, Oct 23, 2018 at 1:06 PM Dewayne Richardson <[email protected]> > wrote: > > > > > > +1 based upon the fact that the Changelog.md can't seem to stay > current, we > > > do need to be able to maybe at least generate one from the commit > messages > > > within that release window. > > > > > > -Dew > > > > > > On Tue, Oct 23, 2018 at 12:58 PM Gray, Jonathan < > [email protected]> > > > wrote: > > > > > > > +1 > > > > > > > > In a past life when using Microsoft VSTS with git, the default > action was > > > > squash. The rule of thumb we used was squash by default and only > merge if > > > > every commit was meaningful and required to understand what you were > trying > > > > to do. We didn't typically have communal feature branches, so it > wasn't as > > > > large a use case for us. The repo forensics are nice with merge, > but the > > > > reality we found was that git blame was more useful when we could > track to > > > > the PR rather than a mid-stream commit and then having to make a > second > > > > jump to the PR from there. It also cut down on back/forth design > change > > > > commits and helped us stay on track with 1 PR == 1 issue because it > made > > > > unrelated fixes more apparent. > > > > > > > > Jonathan G > > > > > > > > > > > > On 10/23/18, 12:40 PM, "Fieck, Brennan" <[email protected]> > > > > wrote: > > > > > > > > I'm not gonna say +1 or -1, but I'd still like to chime in. I > think > > > > it'd be nice to require PRs to squash things into atomic chunks. No > commits > > > > that say "started x" or "minor fixes", but idk if it's reasonable to > be > > > > able to boil every PR forever down to 70 characters. And sure you > can make > > > > multiple PRs if changes are too big to fit in one commit message, > but then > > > > it becomes really easy to become entwined in a chain of dependent > PRs so > > > > that every time one of them gets merged you have to rebase them all. > > > > > > > > Of course, a "reasonable squash" is pretty hard to define > absolutely, > > > > and could actually just make PRs harder to review - maybe it's not > even > > > > enforceable at all. > > > > ________________________________________ > > > > From: Rawlin Peters <[email protected]> > > > > Sent: Tuesday, October 23, 2018 11:14 AM > > > > To: [email protected] > > > > Subject: Re: [EXTERNAL] Re: Squash and Merge for PRs > > > > > > > > +1 on enabling squash and merge. > > > > > > > > This will not only make cherry picks easier to manage with > respect to > > > > merge conflicts, but it will also make the git log more useful > because > > > > we will be able to see the full changeset of the PR in a single > commit > > > > of the git log. Ideally, I'd also like the squashed commit to > contain > > > > a link to the actual github PR so that I can easily view > comments, but > > > > that is a proposal for another day. > > > > > > > > - Rawlin > > > > > > > > On Tue, Oct 23, 2018 at 10:41 AM Dave Neuman <[email protected]> > > > > wrote: > > > > > > > > > > This seems to have stalled, so let me try to move it along. > > > > > > > > > > There is an ability to do "squash and merge" in github, we > just need > > > > to > > > > > agree that we want to enable it for our project. > > > > > > > > > > @Jeremy Mitchell <[email protected]> I am not sure > exactly how > > > > to > > > > > handle the multiple committers thing, but I assume we can > squash > > > > into two > > > > > commits and give one to each. However, if we are doing > something big > > > > > enough to require that many commits from multiple users, it > may be > > > > best as > > > > > many PRs? I don't think this is a very common situation, so I > would > > > > like > > > > > to avoid getting bogged down in what-ifs if possible. > > > > > > > > > > As for the Changelog, I am fine with enabling squash and merge > and > > > > keeping > > > > > the changelog as-is while we decide if squash and merge meets > our > > > > needs. I > > > > > am also game to revisit different ways to do it, and I > shouldn't have > > > > > conflated that conversation with this one. If someone wants > to see > > > > that > > > > > changed, we should start a new discussion. > > > > > > > > > > Leif brings up a good point about squash and merge making > cherry > > > > picks > > > > > easier so I think we should consider it for that point alone. > > > > > > > > > > So, what I am really asking is: "do we think it is a good > idea to > > > > enable > > > > > squash and merge on our Github repo so that we can reduce the > amount > > > > of > > > > > commits per PR?" I would like to get consensus for or > against. If > > > > it is > > > > > not clear already, I am for. > > > > > > > > > > > > > > > Thanks, > > > > > Dave > > > > > > > > > > > > > > > On Thu, Oct 18, 2018 at 2:41 PM Gelinas, Derek < > > > > [email protected]> > > > > > wrote: > > > > > > > > > > > I'll be honest, I think the simplest thing is still to just > > > > require a > > > > > > changelog message, added to a central file, to be included > in the > > > > commit. > > > > > > We've been going over and over this for months instead of > actually > > > > writing > > > > > > change logs. Let's just propose it and put it to a vote. > > > > > > > > > > > > > On Oct 18, 2018, at 4:33 PM, Rawlin Peters < > > > > [email protected]> > > > > > > wrote: > > > > > > > > > > > > > > I'm +1 on squashing and merging, but I think we need a > > > > combination of > > > > > > > a number of things to really have a decent changelog. Even > if we > > > > have > > > > > > > awesome commit messages and squashed PRs, there's still > way too > > > > many > > > > > > > PRs in a release to just generate a decent changelog from > the PR > > > > > > > titles and/or commit messages. We still need a way to > logically > > > > > > > separate PRs into bugfixes, new features, breaking > changes, etc. > > > > We > > > > > > > could get some of that separation using github tags, but > what if > > > > there > > > > > > > are multiple PRs that compose one new high-level feature? > At that > > > > > > > point I think we'd have to create a new tag for every new > > > > high-level > > > > > > > feature being worked on in order to group those PRs under > the > > > > same > > > > > > > heading. There's also the problem that only committers can > tag > > > > issues, > > > > > > > so it adds to the burden of committers to make sure all > > > > non-committer > > > > > > > issues/PRs are properly tagged. > > > > > > > > > > > > > > The main headache with doing a manually-curated changelog > is the > > > > merge > > > > > > > conflicts it causes, but I think we can easily avoid that > > > > problem by > > > > > > > using a similar approach to reno [1] which breaks > individual > > > > notes out > > > > > > > into their own separate yaml files which are then parsed > and > > > > merged > > > > > > > into a single, cohesive changelog for each release. What > that > > > > means is > > > > > > > basically every logical bugfix/new feature/upgrade note > gets its > > > > own > > > > > > > file which avoids merge conflicts caused by just adding a > line > > > > to the > > > > > > > end of a CHANGELOG.md file every time. > > > > > > > > > > > > > > - Rawlin > > > > > > > > > > > > > > [1] https://docs.openstack.org/reno/latest/user/usage.html > > > > > > >> On Thu, Oct 18, 2018 at 1:45 PM Jeremy Mitchell < > > > > [email protected]> > > > > > > wrote: > > > > > > >> > > > > > > >> is there a setting in github to enable "squash and merge" > vs > > > > "rebase and > > > > > > >> merge"? > > > > > > >> > > > > > > >> i do have a couple of concerns however: > > > > > > >> > > > > > > >> 1. what happens when 2+ people contribute to a PR? This > is a > > > > pretty > > > > > > common > > > > > > >> scenario. If Bob and Sam both work on PR#1 is all the > commit > > > > history for > > > > > > >> Sam lost? > > > > > > >> 2. i can see benefits of having granular history...to a > point... > > > > > > >> > > > > > > >> The more I think about this. Why is squash needed? > Commits are > > > > grouped > > > > > > by > > > > > > >> PRs. Why isn't our change log just a list of PR merged > since > > > > the last > > > > > > >> release? If a PR represents a working unit (as it > should), PR > > > > titles > > > > > > should > > > > > > >> be sufficient, no? > > > > > > >> > > > > > > >> For example, this is what our internal release change log > looks > > > > like. > > > > > > >> > > > > > > >> Changelog from 2526569a to 284555703 > > > > > > >> ======================================== > > > > > > >> > > > > > > >> These changes are deployed in branch deploy-20181002. > > > > > > >> > > > > > > >> PR 2300: Add TO Go cachegroups/id/deliveryservices > > > > > > >> 65e27d2cb7c12b25c97bc98b09ffa6577bb6e1ff: Add TO Go > > > > > > >> cachegroups/id/deliveryservices (Robert Butts, May 18 > 2018) > > > > > > >> 130baa85b122f2827621c98f9e87b3245e18d80b: Add TO Go > > > > > > cachegroups/dses > > > > > > >> API test, client funcs (Robert Butts, Jun 28 2018) > > > > > > >> 5197a2da7e323976d5404ce135ff8c42cbed64ef: Remove TO > > > > unused func > > > > > > >> (Robert Butts, Sep 13 2018) > > > > > > >> PR 2803: Add Traffic Ops Golang Steering Targets > > > > > > >> 8ba7d24303d2b420d5059654f83a7154ff7a0703: Add TO Go > > > > > > >> steering/id/targets (Robert Butts, Jul 10 2018) > > > > > > >> 5ba9a3b40b59f6de2c40ad71c5a0a1a84d3acacf: Add TO > API > > > > Tests for > > > > > > >> steeringtargets (Robert Butts, Sep 11 2018) > > > > > > >> eca0a7eab65cd37d6e4f0658c7640200b2e9ecda: Add TO Go > > > > client > > > > > > >> steeringtarget funcs (Robert Butts, Sep 12 2018) > > > > > > >> PR 2806: Add sub to validate user input when running > > > > postinstall script > > > > > > >> e50eeb2c7f46828cbe1d461288f2d98ccdd4ebaa: Add sub > to > > > > validate > > > > > > user > > > > > > >> input when running postinstall script changes default cdn > name > > > > to not > > > > > > >> include an underscore. (Dylan Souza, Sep 11 2018) > > > > > > >> 9ac409904987ad0b52e3de26b79422b9688bbb8e: Altering > > > > postinstall > > > > > > cdn > > > > > > >> name regex to include underscores (Dylan Souza, Sep 21 > 2018) > > > > > > >> 70d8893335b63db006974932341378717c42701c: Changing > back > > > > the > > > > > > default > > > > > > >> cdn name value (Dylan Souza, Sep 21 2018) > > > > > > >> PR 2812: remove traffic_monitor_java > > > > > > >> 4e7a7ab26cce0903ebe54dd0892da45feaf8d3de: remove > > > > > > traffic_monitor_java > > > > > > >> (Dan Kirkwood, Sep 12 2018) > > > > > > >> > > > > > > >> > > > > > > >> [truncated] > > > > > > >> > > > > > > >> > > > > > > >>> On Thu, Oct 18, 2018 at 9:52 AM Dave Neuman < > [email protected]> > > > > wrote: > > > > > > >>> > > > > > > >>> Well, our "curated" changelogs are missing A LOT of > > > > information on what > > > > > > >>> actually went into the release, which therefore makes it > not > > > > useful. > > > > > > If we > > > > > > >>> were better about commit messages and used squash and > merge, > > > > then we > > > > > > would > > > > > > >>> at least have something for every PR that was merged. > > > > > > >>> > > > > > > >>> On Thu, Oct 18, 2018 at 8:25 AM Jan van Doorn < > [email protected]> > > > > wrote: > > > > > > >>> > > > > > > >>>>> I think this just shifts the burden from writing a > changelog > > > > entry to > > > > > > >>>> writing a good commit entry. > > > > > > >>>> > > > > > > >>>> > > > > > > >>>> I agree, and I think that’s where that burden belongs. > (And I > > > > _really_ > > > > > > >>>> hate merge conflicts). > > > > > > >>>> > > > > > > >>>>> There might be fewer commit entries if we squash and > merge, > > > > but I’m > > > > > > >>>> doubtful that it would be as valuable as our “curated” > > > > changelogs. > > > > > > >>>> > > > > > > >>>> Both are dependent on how disciplined we are. > > > > > > >>>> > > > > > > >>>> I’m +1 on Squash and Merge. > > > > > > >>>> > > > > > > >>>> Cheers, > > > > > > >>>> JvD > > > > > > >>>> > > > > > > >>>> > > > > > > >>>>> On Oct 18, 2018, at 08:18, Eric Friedrich (efriedri) > > > > > > >>>> <[email protected]> wrote: > > > > > > >>>>> > > > > > > >>>>> I think this just shifts the burden from writing a > changelog > > > > entry to > > > > > > >>>> writing a good commit entry. > > > > > > >>>>> > > > > > > >>>>> There might be fewer commit entries if we squash and > merge, > > > > but I’m > > > > > > >>>> doubtful that it would be as valuable as our “curated” > > > > changelogs. > > > > > > >>>>> > > > > > > >>>>> > > > > > > >>>>> > > > > > > >>>>> > > > > > > >>>>>> On Oct 18, 2018, at 9:40 AM, Dave Neuman < > [email protected]> > > > > wrote: > > > > > > >>>>>> > > > > > > >>>>>> Hey All, > > > > > > >>>>>> I want to follow up on something that was briefly > discussed > > > > at the > > > > > > >>>> summit > > > > > > >>>>>> this week. Many people do not like the Changelog and > feel > > > > like it > > > > > > can > > > > > > >>>> be a > > > > > > >>>>>> PITA to deal with. One of the reasons we have the > > > > changelog is > > > > > > >>> because > > > > > > >>>>>> there are so many commits in a given release, that it > is > > > > hard to > > > > > > comb > > > > > > >>>>>> through all of them to figure out what noteworthy > changes > > > > or bug > > > > > > fixes > > > > > > >>>> went > > > > > > >>>>>> into the code. One thing that may help with this > problem > > > > is to use > > > > > > >>>> enable > > > > > > >>>>>> the squash and merge feature on Github for pull > requests > > > > [1]. This > > > > > > >>>>>> feature would squash all commits in a PR into one > commit. > > > > If we > > > > > > pair > > > > > > >>>> the > > > > > > >>>>>> one commit with a good commit message, we would > essentially > > > > get the > > > > > > >>>> ability > > > > > > >>>>>> to create the changelog just from the commits. > > > > > > >>>>>> > > > > > > >>>>>> So, I would like to propose that we enable the squash > and > > > > merge > > > > > > >>> feature > > > > > > >>>> in > > > > > > >>>>>> the Traffic Control Github repo and use that going > forward. > > > > > > Thoughts? > > > > > > >>>>>> > > > > > > >>>>>> Thanks, > > > > > > >>>>>> Dave > > > > > > >>>>>> > > > > > > >>>>>> > > > > > > >>>>>> [1] > > > > https://help.github.com/articles/about-pull-request-merges/ > > > > > > >>>>> > > > > > > >>>> > > > > > > >>>> > > > > > > >>> > > > > > > > > > > > > > > > > > > >
