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

[ci skip] Add a note that puma-plugin-systemd is redundant now #3120

Merged
merged 1 commit into from Apr 7, 2023

Conversation

ekohl
Copy link
Contributor

@ekohl ekohl commented Apr 5, 2023

Description

By now Puma has gained all systemd integrations that puma-plugin-systemd used to provide. While I didn't check, there's a good chance the plugin now conflicts with the integrated features conflict. Users should prefer native support.

Your checklist for this pull request

  • I have reviewed the guidelines for contributing to this repository.
  • I have added (or updated) appropriate tests if this PR fixes a bug or adds a feature.
  • My pull request is 100 lines added/removed or less so that it can be easily reviewed.
  • If this PR doesn't need tests (docs change), I added [ci skip] to the title of the PR.
  • If this closes any issues, I have added "Closes #issue" to the PR description or my commit messages.
  • I have updated the documentation accordingly.
  • All new and existing tests passed, including Rubocop.

README.md Outdated
@@ -375,7 +375,7 @@ Community guides:

* [puma-metrics](https://github.com/harmjanblok/puma-metrics) — export Puma metrics to Prometheus
* [puma-plugin-statsd](https://github.com/yob/puma-plugin-statsd) — send Puma metrics to statsd
* [puma-plugin-systemd](https://github.com/sj26/puma-plugin-systemd) — deeper integration with systemd for notify, status and watchdog
* [puma-plugin-systemd](https://github.com/sj26/puma-plugin-systemd) — deeper integration with systemd for notify, status and watchdog; now all integrated in Puma itself
Copy link
Member

Choose a reason for hiding this comment

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

What about removing the link? Or quantify 'now' by stating from which Puma version everything is included

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I also considered removal, but no idea about what's desired.

Copy link
Member

Choose a reason for hiding this comment

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

Any of my suggestions are desired :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removing is best for the reader, because it's no longer relevant. It doesn't give credit to the original author, whose work was a big inspiration for the native integration I did, but I wonder if that's a concern. If you go to https://puma.io/puma4/ it will still have the README from that version, mentioning the plugin so adding a version isn't that useful IHMO.

So the question comes down to credit. Do we feel this is needed, and if so how? Perhaps a code comment in lib/puma/plugin/systemd.rb?

Copy link
Member

Choose a reason for hiding this comment

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

Let's just remove the link

Copy link
Member

Choose a reason for hiding this comment

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

Could also say "Unnecessary since <PUMA_VER> and remove later".

Copy link
Member

Choose a reason for hiding this comment

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

I think I would be in favor of saying "not necessary since VERSION" because people reading the README are not necessarily using the current Puma version, since they're often on Github viewing the master branch. We can remove the link in a year or two.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looking at the changelog, 5.1.0, 5.3.0 and 6.1.0 were significant releases. I've updated the message so please review if this addresses your concerns.

Copy link
Member

Choose a reason for hiding this comment

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

Wonderful! Very informative.

By now Puma has gained all systemd integrations that puma-plugin-systemd
used to provide. While I didn't check, there's a good chance the plugin
now conflicts with the integrated features conflict. Users should prefer
native support.
@nateberkopec nateberkopec merged commit 210ad1e into puma:master Apr 7, 2023
12 checks passed
@ekohl ekohl deleted the update-readme branch April 7, 2023 08:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants