-
-
Notifications
You must be signed in to change notification settings - Fork 9.7k
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
service: support multiple sockets in DSL #16026
Conversation
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.
There was a problem hiding this 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.
docs/Formula-Cookbook.md
Outdated
```rb | ||
service do | ||
run [opt_bin/"beanstalkd", "test"] | ||
sockets "Socket" => "tcp://0.0.0.0:80", "SocketTLS" => "tcp://0.0.0.0:443" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 😁
There was a problem hiding this comment.
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.
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 |
I feel like if we're writing the XML using a ReXML: this stuff will be handled for us. |
Of course, brain fart. |
18e0144
to
7b1c990
Compare
There was a problem hiding this 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.
docs/Formula-Cookbook.md
Outdated
```rb | ||
service do | ||
run [opt_bin/"beanstalkd", "test"] | ||
sockets socket: "tcp://0.0.0.0:80", socket_tls: "tcp://0.0.0.0:443" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
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"] |
There was a problem hiding this comment.
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.
7b1c990
to
5fb9f90
Compare
There was a problem hiding this 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!
brew style
with your changes locally?brew typecheck
with your changes locally?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 ofListeners
to go along with lowercase Ruby symbol naming conventions.CC: @SMillerDev