Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Generate matrix env entries #13

Merged
merged 17 commits into from
Oct 26, 2022
Merged

Generate matrix env entries #13

merged 17 commits into from
Oct 26, 2022

Conversation

meatballhat
Copy link
Member

@meatballhat meatballhat commented Oct 22, 2022

(and arguably too many other cleanups along the way)

Closes #12
Closes #14
Closes #15

@meatballhat meatballhat marked this pull request as ready for review October 23, 2022 14:08
@meatballhat meatballhat requested a review from a team October 23, 2022 14:08
@meatballhat meatballhat marked this pull request as draft October 23, 2022 22:38
@meatballhat meatballhat marked this pull request as ready for review October 24, 2022 01:20
Copy link
Contributor

@philpennock philpennock left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, with caveat of a misleading/false comment being inserted.

Every other nit I had was already present in the code, and I don't go for "make the person who touches it clean up all past mistakes at the same time".

.testdata/sample-versions.txt Outdated Show resolved Hide resolved
Copy link
Contributor

@brackendawson brackendawson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm happy with this as is, but have a few comments you might like to address at this time.

Also what is urfave/gimme's policy for commits/messages? Personally, I would normally squash this if I was merging. Or I'd ask the author to squash it down to one commit per issue being addressed, but I don't think that's possible in this case as they are quite intertwined. CI changes are so hard to get right without multiple attempts. 😆

internal/matrix.go Show resolved Hide resolved
internal/matrix.go Outdated Show resolved Hide resolved
@meatballhat
Copy link
Member Author

Also what is urfave/gimme's policy for commits/messages? Personally, I would normally squash this if I was merging. Or I'd ask the author to squash it down to one commit per issue being addressed, but I don't think that's possible in this case as they are quite intertwined

@brackendawson I think we're in the process of defining what the gimme policies/conventions/norms are right now 🤘🏼 ❤️

I personally try to keep the messy bits of the commit history as part of what's re-integrated into main as much as possible because:

  • it happened, it's part of the story of how we got here (avoiding the myth of the genius programmer)
  • the summary diff at the PR/branch level is more important to me than each commit
  • GitHub automatic release notes are lovely, but they want merge commits
  • ignoring merge commits when desirable is a thing

All of that being said, and to your point about getting CI changes right, this is more messy than what I'd typically be comfortable merging 😂 What do you think about me pushing a selectively squashed rebase that trims down the CI change attempts?

meatballhat added a commit that referenced this pull request Oct 25, 2022
Copy link
Contributor

@brackendawson brackendawson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd be happy with squashing the re-tries into the commits before them. Also giving my ✅ here anyway in case that turns out to be non-trivial. In lieu of a CONTRIBUTING.md to the contrary I don't think one can justify requesting changes to the number of commits.

meatballhat added a commit that referenced this pull request Oct 26, 2022
@meatballhat
Copy link
Member Author

@brackendawson selective squashing done 👍🏼

Copy link
Contributor

@brackendawson brackendawson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@brackendawson brackendawson merged commit 2265ff7 into main Oct 26, 2022
@brackendawson brackendawson deleted the matrixize branch October 26, 2022 19:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants