13 ms·

As technical lead for the OpenTF project, how does things like this get merged? https://github.com/opentffoundation/opentf/pull/36/commits https://github.com/o

As technical lead for the OpenTF project, how does things like this get merged? https://github.com/opentffoundation/opentf/pull/36/commits https://github.com/opentffoundation/opentf/pull/36/commits

lucasyvas 3y ago
Give them some time to figure out what merge strategy they want to enforce - it was probably overlooked as a repository setting and they probably clicked Merge instead of Squash and Merge by accident.
- Foxboron 3y ago
  How does the merge setting solve the complete lack of any useful information in the pull request?
  - sanderjd 3y ago
    Your comments here should go in the textbook for "why people often prefer to work in private for awhile while zeroing in on their processes for a new endeavor". 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).
    - Foxboron 3y ago
      Most people don't spend this much time on PR before releasing their fork though. 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.
      - sanderjd 3y ago
        Oh I see now! Your comments about the merits of the pull requests are just bad faith; you're actually making this totally different point about the project that has absolutely nothing to do with squashing or commit messages. Thanks for clearing that up.
        Conan_Kudo 3y ago
        It was definitely not bad faith. They're pointing out that OpenTF isn't bothering to hold to quality standards that Hashicorp had for Terraform before the fork and they aren't trying to raise beyond those standards to reach the levels common in higher quality community projects like Linux, Kubernetes, LibreOffice, and OBS Studio. A project being run by professional developers who have a lot of experience working with the Terraform code should be capable of doing this.
        sanderjd 3y ago
        Well, I disagree that it's not bad faith. 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.
- tech_tuna 3y ago
  Agreed. . . like none of us have ever been in crunch mode. JFC.
yjftsjthsd-h 3y ago
Things like what? That looks like a straightforward rename
- Reventlov 3y ago
  Commit messages like "more", "rollback", "missed that" are not really what you expect from such an organization, so, yeah, that should've been squashed with a descriptive and useful commit message. My repos look like this but… I'm not a professional developer.
  - linuxdude314 3y ago
    [flagged]
    - fishnchips 3y ago
      > were too lazy or hostile to negotiate a deal with Hashicorp And how do you know about any dealings any of us did or didn't have with them, now or in the past? Are you privy to any of that? > lack of competency in execution, planning, strategy, and software engineering One of my old bosses taught me a lesson. Any time you criticize something, you spend your capital. This capital is earned by building value, by being constructive. If you have any constructive feedback, now is the time to speak up or remain silent forever.
- OJFord 3y ago
  I think that's exactly it, a 'straightforward rename' shouldn't consist of 20 commits with crap messages including 'missed that' (+ merge commit). 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.
  - yjftsjthsd-h 3y ago
    I mean, I guess? Some people like small commits, and most of them looked reasonable to me (renaming one thing at a time); criticizing commit style seems like bike-shedding, anyways.
fishnchips 3y ago
Good point, we should enforce squash merging.
- Foxboron 3y ago
  It would be trivial to at least continue the standards set by the terraform project. Now there are commits messing with `internal/backend` that breaks tests with the commit message "more". Someone is going to hit this with `git bisect` and it wont be their lucky day.
  - fishnchips 3y ago
    Fair point, I think we can fix that. Thanks for highlighting that.
    - linuxdude314 3y ago
      [flagged]
      - jefftk 3y ago
        Be nice! They're just taking this on, and not already having sorted out whether they'll work with "all PR commits must have good messages and pass tests" vs "all PRs must be squashed" is the kind of minor issue you see with teams starting this sort of thing.
      - joshmanders 3y ago
        Ah yes, not having a compatible license that won't cause you legal troubles isn't why people pay for software its... checks notes the commit messages used in a PR.
      - laurels-marts 3y ago
        There's a difference between constructive feedback and toxic. Your comment falls in the latter category.
      - 3y ago
cube2222 3y ago
Even though you're being downvoted I do agree that this should've been squashed (I don't see any other problems here, if that's not it). 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!
- Foxboron 3y ago
  The squash merge is not going to solve the lack of proper commit messages and the fact that things are breaking the test suite left, and right. Figuring out bugs with `git bisect` is not going to be a fun endeavour for people trying to understand incompatible changes.
  - cube2222 3y ago
    > and the fact that things are breaking the test suite left, and right 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.
    - Foxboron 3y ago
      > Branch protection doesn't allow merging without a passing test-suite. https://github.com/opentffoundation/opentf/pull/243 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.
      - jefftk 3y ago
        > you will be hitting one of those 19 commits at some point during a bisect 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.
bradleybuda 3y ago
Should have factored out the project name into a build step to make things easier for the next fork
lmm 3y ago
Looks like a good PR to me. It accomplishes something useful, and the fine-grained commits are very helpful for automated bisect (and frankly, if you're not doing automated bisect then what are you even bothering with a VCS for).
dev_0 3y ago
[dead]

As technical lead for the OpenTF project, how does things like this get merged? https://github.com/opentffoundation/opentf/pull/36/commits https://github.com/opentffoundation/opentf/pull/36/commits

lucasyvas 3y ago

Give them some time to figure out what merge strategy they want to enforce - it was probably overlooked as a repository setting and they probably clicked Merge instead of Squash and Merge by accident.

Foxboron 3y ago
How does the merge setting solve the complete lack of any useful information in the pull request?
- sanderjd 3y ago
  Your comments here should go in the textbook for "why people often prefer to work in private for awhile while zeroing in on their processes for a new endeavor". 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).
  - Foxboron 3y ago
    Most people don't spend this much time on PR before releasing their fork though. 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.
    - sanderjd 3y ago
      Oh I see now! Your comments about the merits of the pull requests are just bad faith; you're actually making this totally different point about the project that has absolutely nothing to do with squashing or commit messages. Thanks for clearing that up.
      - Conan_Kudo 3y ago
        It was definitely not bad faith. They're pointing out that OpenTF isn't bothering to hold to quality standards that Hashicorp had for Terraform before the fork and they aren't trying to raise beyond those standards to reach the levels common in higher quality community projects like Linux, Kubernetes, LibreOffice, and OBS Studio. A project being run by professional developers who have a lot of experience working with the Terraform code should be capable of doing this.
        sanderjd 3y ago
        Well, I disagree that it's not bad faith. 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.
tech_tuna 3y ago
Agreed. . . like none of us have ever been in crunch mode. JFC.

yjftsjthsd-h 3y ago

Things like what? That looks like a straightforward rename

Reventlov 3y ago
Commit messages like "more", "rollback", "missed that" are not really what you expect from such an organization, so, yeah, that should've been squashed with a descriptive and useful commit message. My repos look like this but… I'm not a professional developer.
- linuxdude314 3y ago
  [flagged]
  - fishnchips 3y ago
    > were too lazy or hostile to negotiate a deal with Hashicorp And how do you know about any dealings any of us did or didn't have with them, now or in the past? Are you privy to any of that? > lack of competency in execution, planning, strategy, and software engineering One of my old bosses taught me a lesson. Any time you criticize something, you spend your capital. This capital is earned by building value, by being constructive. If you have any constructive feedback, now is the time to speak up or remain silent forever.
OJFord 3y ago
I think that's exactly it, a 'straightforward rename' shouldn't consist of 20 commits with crap messages including 'missed that' (+ merge commit). 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.
- yjftsjthsd-h 3y ago
  I mean, I guess? Some people like small commits, and most of them looked reasonable to me (renaming one thing at a time); criticizing commit style seems like bike-shedding, anyways.

fishnchips 3y ago

Good point, we should enforce squash merging.

Foxboron 3y ago
It would be trivial to at least continue the standards set by the terraform project. Now there are commits messing with `internal/backend` that breaks tests with the commit message "more". Someone is going to hit this with `git bisect` and it wont be their lucky day.
- fishnchips 3y ago
  Fair point, I think we can fix that. Thanks for highlighting that.
  - linuxdude314 3y ago
    [flagged]
    - jefftk 3y ago
      Be nice! They're just taking this on, and not already having sorted out whether they'll work with "all PR commits must have good messages and pass tests" vs "all PRs must be squashed" is the kind of minor issue you see with teams starting this sort of thing.
    - joshmanders 3y ago
      Ah yes, not having a compatible license that won't cause you legal troubles isn't why people pay for software its... checks notes the commit messages used in a PR.
    - laurels-marts 3y ago
      There's a difference between constructive feedback and toxic. Your comment falls in the latter category.
    - 3y ago

cube2222 3y ago

Even though you're being downvoted I do agree that this should've been squashed (I don't see any other problems here, if that's not it). 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!

Foxboron 3y ago
The squash merge is not going to solve the lack of proper commit messages and the fact that things are breaking the test suite left, and right. Figuring out bugs with `git bisect` is not going to be a fun endeavour for people trying to understand incompatible changes.
- cube2222 3y ago
  > and the fact that things are breaking the test suite left, and right 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.
  - Foxboron 3y ago
    > Branch protection doesn't allow merging without a passing test-suite. https://github.com/opentffoundation/opentf/pull/243 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.
    - jefftk 3y ago
      > you will be hitting one of those 19 commits at some point during a bisect 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.

bradleybuda 3y ago

Should have factored out the project name into a build step to make things easier for the next fork

lmm 3y ago

Looks like a good PR to me. It accomplishes something useful, and the fine-grained commits are very helpful for automated bisect (and frankly, if you're not doing automated bisect then what are you even bothering with a VCS for).

dev_0 3y ago

[dead]