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

Add built-in systemd notify support #3011

Merged
merged 1 commit into from Jan 3, 2023
Merged

Add built-in systemd notify support #3011

merged 1 commit into from Jan 3, 2023

Conversation

joaomarcos96
Copy link
Contributor

@joaomarcos96 joaomarcos96 commented Nov 4, 2022

Description

This PR aims to remove the need to install the sd_notify gem by vendoring its code, thus, making the support for systemd notify built-in.

I have considered what @mperham have done with Sidekiq and said about sd_notify #2438 (comment):

With Sidekiq I decided to vendor the code for the sd_notify gem because it's only ~50 lines of stable code. I didn't want to require the user to add the gem to their gemfile manually, that shouldn't be something they need to care about IMO.

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. PS.: I haven't added tests as the sd_notify gem code was copied exactly as it is.
  • 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.

@joaomarcos96
Copy link
Contributor Author

I wasn't able to identify why there are some tests failing. Could someone help me with that?

@MSP-Greg
Copy link
Member

MSP-Greg commented Nov 4, 2022

Re the macOS failures, it looks like line 115 in test_integration_single.rb needs to change to:

assert_match(/Connection refused|Couldn't connect to server/, rejected_curl_stderr.read)

Or, the error message reported by curl has changed?

Let me check...

@joaomarcos96
Copy link
Contributor Author

@MSP-Greg macOS-11 2.6 test is still failing. Any idea why?

@MSP-Greg
Copy link
Member

MSP-Greg commented Nov 8, 2022

macOS-11 2.6 test is still failing

Nothing to do with this PR. Intermittent test failure from hell.

I just noticed something re comments in sd_notify.rb. Most doc systems will attach the comments below require "socket" to the Puma namespace. So, they should probably be moved so they're below module Puma and indented. Doing so will attach them to Puma::SdNotify.

Maybe CI will pass...

@joaomarcos96
Copy link
Contributor Author

@MSP-Greg thank you! I did what you suggested and now all tests are passing.
If this PR gets merged, I'd like the changes to be ported to 5.x as well. How could I do that? Should I open a PR for each 5.x-stable branch?

@dentarg
Copy link
Member

dentarg commented Nov 8, 2022

I'm not sure how clear routines we have for backports (almost never happens in my experience) but https://github.com/puma/puma/tree/5-6-stable looks to be the same as https://github.com/puma/puma/releases/tag/v5.6.5 (I draw this conclusion from the release saying 139 commits to master since this release and the branch saying This branch is 23 commits ahead, 139 commits behind master. => both says 139 ✔️ 😄 – you may want to double check them)

Then there is this branch https://github.com/puma/puma/tree/5-6-5 This branch is 1 commit ahead, 77 commits behind master. not sure why it exist and if it serves any purpose

I think @nateberkopec can provide more guidance on backporting

@nateberkopec
Copy link
Member

If this PR gets merged, I'd like the changes to be ported to 5.x as well. How could I do that? Should I open a PR for each 5.x-stable branch?

@joaomarcos96 Yes, that is correct. In this case though, because we're adding a feature, we should probably create a new minor version 5.7, so if you're still interested LMK and I will create a branch.

@joaomarcos96
Copy link
Contributor Author

@nateberkopec Yes, I'd like that.

@nateberkopec
Copy link
Member

👍 We now have a 5-7-stable branch, you may make PRs against it and we will release @joaomarcos96.

@nateberkopec nateberkopec merged commit 6d8b728 into puma:master Jan 3, 2023
joaomarcos96 added a commit to joaomarcos96/puma that referenced this pull request Jan 4, 2023
kidhab added a commit to foodcoops/sharedlists that referenced this pull request May 2, 2023
kidhab added a commit to foodcoops/sharedlists that referenced this pull request May 2, 2023
@voxik
Copy link
Contributor

voxik commented Jan 9, 2024

As a Fedora maintainer, I am disappointed with this PR and with bundling in general. What was the reason to not specify the dependency in .gemspec? If at least the code was put into dedicated directory to make this more obvious. And if at least the license field was updated to list the MIT license alongside the BSD-3-Clause.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants