As technical lead for the OpenTF project, how does things like this get merged?
I've made sure via repo config that only squash commits are enabled from now on, so this will not happen again. Thanks for the feedback!
Figuring out bugs with `git bisect` is not going to be a fun endeavour for people trying to understand incompatible changes.
Branch protection doesn't allow merging without a passing test-suite.
> The squash merge is not going to solve the lack of proper commit messages
Could you expand? You choose a sensible commit message on squash, while the PR's commits become fairly irrelevant at that point.
https://github.com/opentffoundation/opentf/pull/243
EDIT: and just to point out. If you have 1 PR with 19 commits that break the test suite. The last commit fixing it doesn't matter as you will be hitting one of those 19 commits at some point during a bisect.
>Could you expand? You choose a sensible commit message on squash, while the PR's commits become fairly irrelevant at that point.
It's optional. Nothing prevents you from just adopting whatever the PR said initially. Turning it on doesn't automatically make it better.
But not if you merge the PR as a squash-merge, which turns those 19 "development" commits into a single "permanent" commit.
Using PRs as the unit of development, with all intra-PR work squashed into a single atomic test-passing commit, is a well functioning process that many teams use.
The test suite did its job by alerting you to make changes before merging your work to the trunk branch, I don't see this as anything anybody could possibly pick a fight over.
And to be clear, I still use mainline terraform because I never really wanted a wrapper around terraform. It's just silly to me to take issue with the terraform wrappers for this non-issue.
Imagine you are a terraform provider developer that is working on making sure their code is working with `opentf`.
Lets imagine opentf does an initial `v1.0.0` release of their code and your provider doesn't work. But you know it worked with the last FOSS release of `terraform`.
What do you do?
You find the common ancestor between these two projects, lets say 8a085b427b74ce3829500a59508b77465f1bbef0 (as that is the last commit `opentf` has from `terraform`).
You will now run `git bisect` on the history between `8a085b427b74ce3829500a59508b77465f1bbef0` and `v1.0.0`.
You will do a binary search on the 100-200 commits here, and everytime the source fails to build, or the test suite doesn't pass for whatever reason, you are making it much harder for the downstream provider to figure out why their code doesn't work.
You can easily just try do this today and see what happens. Does the current untidy git history cause you any problems?
I really recommend adopting this strategy.
Don't forget, a project like Terraform is too big. No one person can know how the whole system works. Trying to look through PRs as a means of debugging is a fools errand. You have to take on a different mindset when working on these large codebases.
Then I'll `git bisect --first-parent`, and either isolate it to your merge, or mark to merge OK. `git bisect` doesn't have to walk your branch.
The "real" solution is making the developers aware of the issue and cleanup up their history before doing an MR.
For a start, it's easy to review, and easier to disect if something breaks.
When messy git history is provided (i.e. a history of the developer fixing their own code that they missed), squashing can be a reasonable fallback. But it's never the best option, IMHO.
It's a separate project from git [0].
And the benefit is that instead of humans spending time deciding whether or not to squash commits in some branch before merging, or being asked to do so by a maintainer, a computer just does it automatically. It's a lot like automatic code formatting; it's a big win to have tools do things automatically, rather than requiring human time and judgement and communication.
They're not independent, they're related. Having multiple levels of "namespace" - repo -> PR -> commit - makes things easier to navigate, just like having multiple levels of folder in your filesystem.
> And the benefit is that instead of humans spending time deciding whether or not to squash commits in some branch before merging, or being asked to do so by a maintainer, a computer just does it automatically.
If you demand that each PR must be a single commit you just force the human to make that decision earlier and more manually.
> If you demand that each PR must be a single commit you just force the human to make that decision earlier and more manually.
How so? I think you may be misunderstanding the suggestion... The pattern is that people can have whatever commits they want on their own branch, but when that branch gets merged into the trunk, all those commits are automatically squashed into a single one. I don't see how that is "forc[ing] the human to make that decision earlier and more manually".
Right, so that means people have to decide whether something's going to be a separate commit immediately when they're working on it, and start a new branch if so. Or they could mess around splitting their branch into smaller branches at the end when they're ready to PR, but that's fiddlier than rebasing to split/merge commits.
I don't know what you mean by making PRs being "fiddler than rebasing to split/merge commits". What is the fiddliness you see?
That's a horrible way to work because when you squash a branch it breaks the history of any other branch that's based on it. So you'd have to do a fiddly rebase when you merge your first branch in.
> I don't know what you mean by making PRs being "fiddler than rebasing to split/merge commits". What is the fiddliness you see?
Imagine I've done a bunch of work in a branch, and then decide I want the contents of that branch to become three commits on master. With your workflow I have to split my branch into three branches, retroactively. That's fiddlier than just turning the commits on my branch into three commits.
It really isn't. Branches are just as cheap as commits. It's super easy to create three branches pointing at three arbitrary commits.
There will always be someone who will gleefully jump down your throat for imperfections.
But on balance it's probably better to work in public anyway and ignore the haters (while also continuously improving processes).
If they had worked on this publicly from the start I'm sure a lot of the current PRs would have generally been of better quality as they could get community input earlier.
A project being run by professional developers who have a lot of experience working with the Terraform code should be capable of doing this.
If your problem with a project is that it advertised itself to its target audience prior to releasing a repository, then a good faith comment would say something like, "My problem with this project is that they have spent more time writing manifestos and blog posts and social media comments so far than they have spent developing strong processes and working on publishing a repository for the project".
But if that's your real problem with a project, then a bad faith comment might look like "This is a bad pull request, how could you let it get merged?". The reason that is in bad faith is that it says nothing about your actual problem with the project, it's just a totally random swipe that you don't even actually care about. If there turns out to be a good answer to the question, like "Oh you're right, we forgot to require squashing pull requests when merging, I've fixed it now!", then instead of recognizing that your complaint has been spoken to, you are likely to simply find further things to criticize, because the first thing wasn't actually your real criticism, it was just a smokescreen.
Now, if you are making this other, better, point that the OpenTF project should have more mature software development practices, at least equivalent to and ideally exceeding the better-known project they have forked from, then yep, I agree with that criticism and do not think it is necessarily being made in bad faith.
However, I would be significantly more sympathetic to that criticism if it were a comment on an article entitled "OpenTF project celebrates one full year since its conception" rather than one entitled "OpenTF repository is now public".
The project is brand new, and they clearly rushed to get out a public repository because of other haters who were giving them crap about taking too long to publish the repository, and their processes clearly haven't matured yet. (Or I dunno, maybe it was even many of the same haters, because again, this is the tendency with people who criticize in bad faith, to just move on to the next random criticism that they don't actually care about.)
So if we get to a year from now and their processes remain immature, then yep, I'm right there with you on your criticism. But I think it's crazy (and, sorry to keep repeating myself: likely in bad faith) to make this criticism of such a new project. There is absolutely no indication that the developers running the project are incapable of having mature processes for the project, from the tiny amount of data available from the tiny amount of time that has elapsed since they announced the project.
I think there's a stronger argument of an X/Y problem in the statement, since they talk about what they see as output vs what they expect to see instead.
And frankly, I am disappointed too, as these people working on this project should have spent all that time understanding how Hashicorp actually handled the codebase and ensured that their own engineers followed the same quality level for development. They're all definitely capable of it, so they should actually do it.
Otherwise it's going to be hard to trust the fork to succeed.
(I do think you may be missing that I was responding to the entire sequence of comments, which started with just a drive-by swipe at pull request merging process, not just the later comments expressing disappointment with the perceived emphasis on marketing over engineering.)
I think your direct expression of disappointment is totally reasonable. I personally think it's also super premature to be disappointed, but it's your prerogative.
But I just think the other commenter's approach to expressing this criticism was way off base.
Someone is going to hit this with `git bisect` and it wont be their lucky day.
Or if not: you're welcome!
But tonnes of people don't care about git hygiene or using tools well in general, GP's in for an exhausting time caring about it much in projects they're not in control of.