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

Support multiple named sockets in service block #15945

Closed
1 task done
owenthereal opened this issue Sep 3, 2023 · 8 comments · Fixed by #16026 or #16063
Closed
1 task done

Support multiple named sockets in service block #15945

owenthereal opened this issue Sep 3, 2023 · 8 comments · Fixed by #16026 or #16063
Labels
features New features in progress Maintainers are working on this outdated PR was locked due to age

Comments

@owenthereal
Copy link

Verification

Provide a detailed description of the proposed feature

It would be nice to declare multiple sockets with names in the service block in a formula. For example:

service do
  sockets "Socket" => "tcp://0.0.0.0:80", "SocketTLS" => "tcp://0.0.0.0:443"
end

What is the motivation for the feature?

Some formulas may expose multiple sockets in a plist file. For example, owenthereal/homebrew-candy@92aada6 tries to convert the old plist function with both port 80 and 443 set in the Sockets plist section but couldn't due to sockets in the service block only allows one entry, although sockets with an s.

How will the feature be relevant to at least 90% of Homebrew users?

This prevents some formula that requires exposing multiple sockets from working. I don't have a number on the number of Homebrew users impacted. But the migration from plist to service breaks some formulas.

What alternatives to the feature have been considered?

I do not know a way. Please share if you have any recommendations.

@MikeMcQuaid
Copy link
Member

Makes sense to me. What do you think @SMillerDev?

@apainintheneck
Copy link
Contributor

I looked at the code yesterday and the sockets stanza seems to only be available for launchd on MacOS right now. I think it's a no-brainer. I'm not sure why it was never added to the systemd on the Linux side though. I'm not sure why we don't allow people to name their sockets either or how useful that would be to add in.

@apainintheneck apainintheneck added the help wanted We want help addressing this label Sep 6, 2023
@SMillerDev
Copy link
Member

Yeah, seems fine to me. I never added it because no formulae I knew of needed it.

@apainintheneck apainintheneck added in progress Maintainers are working on this and removed help wanted We want help addressing this labels Sep 24, 2023
@apainintheneck
Copy link
Contributor

@owenthereal It would be great if you could test out the changes on this branch (#16026) to make sure they solve the problem. Also, I looked at your formula and you'll want to use service do instead of def service when defining a service.

@owenthereal
Copy link
Author

owenthereal commented Sep 28, 2023

@apainintheneck Sorry for not getting back to you sooner. I saw the changes were reverted. I'm happy to help test it when it's ready again. I don't know how to test a branch's code changes. Is there a doc that I could take a look at?

@Bo98 Bo98 reopened this Sep 28, 2023
@apainintheneck
Copy link
Contributor

@owenthereal No worries. I just like to double-check with people who open issues to see if it fits their use case. I'll fix and reopen the original PR in the next few days and ping you so that you can take a look at it when that's ready.

I don't think we have documentation for that but that sounds useful since this comes up from time to time. Brew is just a git repo of Ruby code so it's not terribly complex to check out a feature branch locally for testing. I'll provide instructions for testing it in the PR.

@github-actions github-actions bot added the outdated PR was locked due to age label Nov 7, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 7, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
features New features in progress Maintainers are working on this outdated PR was locked due to age
Projects
None yet
7 participants
@MikeMcQuaid @owenthereal @Bo98 @SMillerDev @apainintheneck and others