-
Notifications
You must be signed in to change notification settings - Fork 3
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
Conversation
There was a problem hiding this 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".
There was a problem hiding this 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. 😆
@brackendawson I think we're in the process of defining what the 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:
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? |
There was a problem hiding this 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.
plus other squashed CI fiddling: - Add a missing fromJson - Whoops actually run gimme - Filter gimme output to appease GITHUB_ENV - Use fromJson in the right place
including porting many Shell and Python things to Go
and trim down the list
and cut down on intermediate and platform-specific lists
so that it isn't re-generated at matrix time from a fresh known versions list
given feedback on #13
5f79962
to
13cdf4b
Compare
@brackendawson selective squashing done 👍🏼 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
(and arguably too many other cleanups along the way)
Closes #12
Closes #14
Closes #15