Cleanup gem setup

[stevenharman]

👋 Hello! Thanks for this great Puma plugin. We're integrating it into our setup and I'm working on a new LogTarget and L2metFormatter, which I am hoping to add upstream. But first I had to get the dev setup working on my machine, which is where the .gitignore-ing of Gemfile.lock came from. I also cleaned up a couple of other small housekeeping items.

I'm packing these into a first PR, and will base my actual changes atop this branch.

Thanks again!

[driv3r]

Nice, still I don't get why Gemfile.lock was an issue? Could you elaborate on it?

[stevenharman]

I don't get why Gemfile.lock was an issue? Could you elaborate on it?

Sure. The gist is, it then locked me into the exact same versions of all transient dependencies as whoever last updated the Gemfile.lock. I was running on a newer Ruby (3.3.0) where one of the transient dependencies (nio4r IIRC) couldn't install. But newer versions of that same dependency could install. So, I could have bundle update and it would have worked, and the Gemfile.lock would have churned.

Also, by removing it, it's giving us arguably better coverage as we're running (in local dev) and testing (on CI) against whatever versions are on the machines, so long as they match the constraints in the .gemspec or Gemfile.

Overall this is pretty typical in libraries as you don't really know exactly which versions of which dependencies someone using the library will need/have. But in an "app" (i.e., a Rails app, etc…), you'd 💯 want the Gemfile.lock because you want everyone, and all deployments, to have the exact same versions of everything.

Does that make sense?

[stevenharman]

@driv3r How are you feeling about this PR? If we could get it merged, it will make #21 a little easer to "see" the changes. And if we can get #21 in, I've got a couple of ideas for some small improvements, but want to wait and open separate, focused PRs for them.

[driv3r]

@stevenharman but Gemfile.lock is not loaded when you install the gem, it's used solely for the sake of having exactly the same development environment from machine to machine. Then it's easier to reason about the issues, instead of figuring out after debugging that a version of particular gem was different.

Same with the ruby version, plus for development you should actually use the lowest supported ruby version, so in the case of this particular gem it would be 2.6.

So, I would say:

• bring back Gemfile.lock
• update it, if necessary - but with gems against ruby 2.6

And I would say we are ready to roll :)

[stevenharman]

@driv3r Fair enough. It turns out that including the Gemfile.lock is now recommended by Bundler. It used to be the opposite. So, TIL! 😆

I've pushed up those changes.

[driv3r]

@soulim looks good from my side 👍

[soulim]

Thank you for fixing Datadog and StatsD names! 😄👍

[unsign3d]

If it's ok for you, I would ask you to create a different PR for the bin files, so that I can approve this PR, and discuss internally about the bin stuff :)

[unsign3d]

I'm not really sure about this change, I think internally we always use bundle exec 🤔
Any reason why wasn't it working for you?

[stevenharman]

The both end up running in the context of Bundler. Partially, binstubs are a habit/ergonomics that I've grown accustomed to. Specifically, there's a normalization of "the tools I need to run for development are in bin/*". The bin/setup and bin/console already live there, for example. And now so do the other tools you'll need day-to-day, like bin/rspec or bin/rake.

But in terms of DX, the existence of various binstubs allows some tooling (think, Vim plugins, VSCode extensions, etc…) to make decisions about how to invoke tooling. For example, a bin/rubocop existing let's Vim's ALE plugin know that it should use Rubocop (and the right version of it by way of the binstub) rather than Standard.rb as the linter/fixer.

You can still use bundle exec even with a binstub existing. But the existence of a binstub both signals "this is tooling expected to be here, and be used by, this code base," and it enables certain developer tooling to work without additional configuration.

All of that said, I can pull this out to a separate PR if you wish. Thoughts?

[stevenharman]

@unsign3d I went ahead and rebased the binstubs changes out. I'd still like to at least get them added, even if we continue to use bundle exec in the GitHub Action - just so local tooling works out of the box. But we can talk about that in another PR, as you've suggested.

[unsign3d]

@stevenharman yeah, thank you, I feel that it would be easier to discuss in a different PR, I don't have strong feelings about either, but I want to try to push things as fast as I can.
I will in the meantime try to educate myself more on the topic :)

[stevenharman]

Sounds good. Once this PR is merged I'll start opening smaller ones for these other changes; they just all depend on this one, so they look larger than they really are. 😄

[unsign3d]

Btw, thank you for contributing :)

[soulim]

LGTM 👍 Thank you!

[unsign3d]

@stevenharman, it seems that the new version of rubocop is complaining, I think you can directly run a rubocop -A and everything should be fixed.

Also would you mind bumping up the version? (you can follow the guide in the Readme) I guess after that I can merge it :)

[stevenharman]

@unsign3d I've appeased the cop 👮‍♂️ and pushed up those changes. 😄

As for bumping the version, we could, or… I was thinking we'd wait until there's actually a behavior change. This PR is really just dev and packaging changes. For example, after #21 I could see changing the version. And I actually have another, smaller, change for the socket telemetry when running on platforms that don't support the technique we're using.

So, maybe we merge this as-is, and wait to bump the version until there's a behavior change?

[stevenharman]

Thanks for all of the feedback and help on this on, @driv3r, @unsign3d, and @soulim. Please let me know if there's anything else I can do to get this one merged; I'm itching to get some more changes built atop this on. 😄

[unsign3d]

@stevenharman I just saw that this behaves slightly different than our internal stuff and we are more flexible with the versioning :)
I will wait for the green build and then merge it 👍

[stevenharman]

this behaves slightly different than our internal stuff

I'm curious - how so?

[stevenharman]

So… I don't think the ubuntu 18.04 Actions are ever going to work. They've been removed. Maybe we can skip them, merge this, and I'll open a new PR to adjust the supported Ubuntu versions? I went ahead and rebased in a change to test against the supported Ubuntu versions: 20.04, 22.04, and latest. That should get this PR to ✅.