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

WIP Gel install gem command #58

Merged
merged 7 commits into from Oct 6, 2019

Conversation

@searls
Copy link
Collaborator

commented May 3, 2019

WIP: Do not merge

  • Write a failing test of the basic general case
  • Make it pass
  • Consider the case of being in a directory with a Gemfile, should we use the locked version instead of latest?
  • Add it to the CLI
  • document the feature

Resolves #52

@matthewd

This comment has been minimized.

Copy link
Member

commented May 5, 2019

The silent-failure-to-install we were seeing yesterday turned out to be a bug in #57 -- the dependencies list looked slightly different depending on whether it was read from a lockfile (the existing behavour used elsewhere) or built from a new resolution (previously only used to write out a lockfile, which would "fix" it).

After shuffling some deckchairs to reduce some of our shortcut hacks, I've now also fixed that bug in this branch. I think there are still some hacks to go: the additional install of pub_grub is the one that comes to mind... it's livable for now, but sets up a bad precedent as we start to build out more CLI-level tests, if every one of them needs to waste time doing that before they can do their actually-interesting work.

This just makes the #work method a bit clearer -- and this isn't real
"work" anyway, as it doesn't involve catalog communication.
@searls

This comment has been minimized.

Copy link
Collaborator Author

commented May 6, 2019

Well that's good.

I think the messy duplication is messy enough that we should try taking a stab at cleaning it up before merging this. Now that RailsConf is behind us I hope we can find the time.

Hey @tenderlove this would be great to pair with you on

@searls

This comment has been minimized.

Copy link
Collaborator Author

commented May 29, 2019

hey @matthewd -- as usual I have fallen off the face of the planet. How far afield is this from where gel is currently? Worth fixing merge conflicts and trying again?

matthewd added a commit that referenced this pull request Oct 6, 2019
Gel install gem command
@matthewd matthewd merged commit 609fab8 into master Oct 6, 2019
1 check passed
1 check passed
buildkite/gel Build #205 passed (4 minutes, 30 seconds)
Details
@matthewd

This comment has been minimized.

Copy link
Member

commented Oct 6, 2019

I managed to fall off the plane of the galaxy, it would seem 😔

I've finally come back to look at this, and it seems to be working fine?! I thought this was still in an unmergeable state, and in need of a concerted chunk of attention, but maybe that's what you get for working on it right before a 17 hour flight.

This does still lack some of the points in the top todo list (the CLI's there, though we might want to consider friendlier spellings: gel install-gem standard Just Works, as far as I can tell), but I think it's strong enough to merge -- and I particularly wanted to get the supporting rearrangements landed.

@searls

This comment has been minimized.

Copy link
Collaborator Author

commented Oct 7, 2019

Yay, this is awesome!

@searls searls deleted the gel-install-gem branch Oct 7, 2019
@searls

This comment has been minimized.

Copy link
Collaborator Author

commented Oct 7, 2019

Feel free to make issues from the remaining nice to haves if you think they're worth prioritizing

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.