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

service: support multiple sockets in DSL #16026

Conversation

apainintheneck
Copy link
Contributor

@apainintheneck apainintheneck commented Sep 24, 2023

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same change?
  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your changes? Here's an example.
  • Have you successfully run brew style with your changes locally?
  • Have you successfully run brew typecheck with your changes locally?
  • Have you successfully run brew tests with your changes locally?

Closes #15945

This adds support for multiple named sockets to the service DSL. It also retains backwards compatibility with the previous DSL where you can declare one socket. The only change is that the default socket name is now listeners instead of Listeners to go along with lowercase Ruby symbol naming conventions.

CC: @SMillerDev

This adds support for multiple named sockets to the service DSL.
It also retains backwards compatibility with the previous DSL
where you can declare one socket and it is always just named
Listener by default.
Copy link
Member

@MikeMcQuaid MikeMcQuaid left a comment

Choose a reason for hiding this comment

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

Great work so far @apainintheneck! Looks good to me with a slight (non-blocking) naming suggestion.

```rb
service do
run [opt_bin/"beanstalkd", "test"]
sockets "Socket" => "tcp://0.0.0.0:80", "SocketTLS" => "tcp://0.0.0.0:443"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
sockets "Socket" => "tcp://0.0.0.0:80", "SocketTLS" => "tcp://0.0.0.0:443"
sockets socket: "tcp://0.0.0.0:80", socket_tls: "tcp://0.0.0.0:443"

or something might be a nicer/more Ruby-like API. Not huge on the socket:/socket_tls: naming, though; would welcome better ones.

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 think using a string here gives users more flexibility when it comes to matching the exact names of the sockets they already use internally. Or at least that was my thinking.

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 using a string here gives users more flexibility when it comes to matching the exact names of the sockets they already use internally.

@apainintheneck Can you elaborate on this a bit? Do the names of these sockets matter and, if so, when/how are they used?

To be clear: this may be entirely just correctly my understanding 😁

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 didn't know what I was talking about 😄.

From the man page man launchd.plist:

Sockets <dictionary of dictionaries... OR dictionary of array of dictionaries...>

This optional key is used to specify launch on demand sockets that can be used to let launchd know when
to run the job. The job must check-in to get a copy of the file descriptors using the
launch_activate_socket(3) API.  The keys of the top level Sockets dictionary can be anything. These
keys are meant for the application developer to associate which socket descriptors correspond to which
application level protocols (e.g. http vs. ftp vs. DNS...).

Looking at it again we use symbols for rest of the service DSL. I'll change the code to use symbols.

@apainintheneck
Copy link
Contributor Author

How worried should we be about validating socket names? They need to fit into the plist so I assume some names are off limits, right? Like for example names including < would break the XML.

@MikeMcQuaid
Copy link
Member

How worried should we be about validating socket names? They need to fit into the plist so I assume some names are off limits, right? Like for example names including < would break the XML.

I feel like if we're writing the XML using a ReXML: this stuff will be handled for us.

@apainintheneck
Copy link
Contributor Author

I feel like if we're writing the XML using a ReXML: this stuff will be handled for us.

Of course, brain fart.

@apainintheneck apainintheneck force-pushed the support-multiple-sockets-in-service-dsl branch from 18e0144 to 7b1c990 Compare September 27, 2023 03:48
Copy link
Member

@MikeMcQuaid MikeMcQuaid left a comment

Choose a reason for hiding this comment

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

Looks great! One more style thought but optional/not blocking merge.

```rb
service do
run [opt_bin/"beanstalkd", "test"]
sockets socket: "tcp://0.0.0.0:80", socket_tls: "tcp://0.0.0.0:443"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
sockets socket: "tcp://0.0.0.0:80", socket_tls: "tcp://0.0.0.0:443"
sockets http: "tcp://0.0.0.0:80", https: "tcp://0.0.0.0:443"

or something perhaps so these names are a bit more meaningful?

alternatively, if we want to just generate the names for people:

Suggested change
sockets socket: "tcp://0.0.0.0:80", socket_tls: "tcp://0.0.0.0:443"
sockets ["tcp://0.0.0.0:80", "tcp://0.0.0.0:443"]

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'll update the names in the docs to be more meaningful like you mentioned. I think allowing names can be helpful for documentation reasons but I don't feel very strongly either way.

This is more in keeping with the other DSL methods and Ruby
convention along with the fact that these socket names are
just used internally by launchd.
@apainintheneck apainintheneck force-pushed the support-multiple-sockets-in-service-dsl branch from 7b1c990 to 5fb9f90 Compare September 28, 2023 05:30
@apainintheneck apainintheneck marked this pull request as ready for review September 28, 2023 05:50
Copy link
Member

@MikeMcQuaid MikeMcQuaid left a comment

Choose a reason for hiding this comment

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

Looks great, thanks again @apainintheneck!

@MikeMcQuaid MikeMcQuaid merged commit 9359f95 into Homebrew:master Sep 28, 2023
25 checks passed
@github-actions github-actions bot added the outdated PR was locked due to age label Oct 29, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 29, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated PR was locked due to age
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support multiple named sockets in service block
2 participants