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: add sockets and keepalive variants #12790

Merged

Conversation

SMillerDev
Copy link
Member

  • 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?

This adds the last remaining features to services before we can deprecate plist usage.

@BrewTestBot
Copy link
Member

Review period will end on 2022-01-26 at 18:30:16 UTC.

@BrewTestBot BrewTestBot added the waiting for feedback Merging is blocked until sufficient time has passed for review label Jan 25, 2022
@SMillerDev
Copy link
Member Author

See Homebrew/homebrew-core#93612

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 good so far!

How many plists still need ported in homebrew-core?

Library/Homebrew/service.rb Show resolved Hide resolved
Library/Homebrew/service.rb Outdated Show resolved Hide resolved
Library/Homebrew/service.rb Outdated Show resolved Hide resolved
@SMillerDev
Copy link
Member Author

These would be the last ones that aren't deprecated.

@BrewTestBot BrewTestBot removed the waiting for feedback Merging is blocked until sufficient time has passed for review label Jan 26, 2022
@BrewTestBot
Copy link
Member

Review period ended.

Library/Homebrew/service.rb Outdated Show resolved Hide resolved
Library/Homebrew/service.rb Outdated Show resolved Hide resolved
@SMillerDev SMillerDev force-pushed the feature/service/socket_and_keepalive branch 3 times, most recently from 3531198 to 11b799a Compare February 23, 2022 07:51
@Bo98
Copy link
Member

Bo98 commented Mar 14, 2022

Just realised that I maybe should have held merging #12988 as I'm not sure how it would work API wise with what you're doing here.

(Though the API can change until a new release is tagged - and I'll hold off doing so today.)

Maybe it's fine - I'm guessing the change would be it would take a parameter for the "type" and it could default to always to allow a no-arg call, unless we want to enforce always specifying the type.

@SMillerDev
Copy link
Member Author

I already spotted the services part of that PR and it's fine.

@github-actions
Copy link

github-actions bot commented Apr 5, 2022

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

@github-actions github-actions bot added the stale No recent activity label Apr 5, 2022
@SMillerDev SMillerDev force-pushed the feature/service/socket_and_keepalive branch 4 times, most recently from 210eab3 to 8c68a67 Compare April 5, 2022 15:44
@SMillerDev
Copy link
Member Author

Okay, this is ready for another round now

@github-actions github-actions bot removed the stale No recent activity label Apr 5, 2022
Library/Homebrew/service.rb Outdated Show resolved Hide resolved
Library/Homebrew/service.rb Outdated Show resolved Hide resolved
Library/Homebrew/service.rb Outdated Show resolved Hide resolved
Comment on lines 353 to 362
if @keep_alive[:always].present?
base[:KeepAlive] = @keep_alive[:always]
elsif @keep_alive.key? :succesful_exit
base[:KeepAlive] = { SuccessfulExit: @keep_alive[:succesful_exit] }
elsif @keep_alive.key? :crashed
base[:KeepAlive] = { Crashed: @keep_alive[:crashed] }
elsif @keep_alive.key? :path
base[:KeepAlive] = { PathState: @keep_alive[:path].to_s }
end
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
if @keep_alive[:always].present?
base[:KeepAlive] = @keep_alive[:always]
elsif @keep_alive.key? :succesful_exit
base[:KeepAlive] = { SuccessfulExit: @keep_alive[:succesful_exit] }
elsif @keep_alive.key? :crashed
base[:KeepAlive] = { Crashed: @keep_alive[:crashed] }
elsif @keep_alive.key? :path
base[:KeepAlive] = { PathState: @keep_alive[:path].to_s }
end
if (always = @keep_alive[:always].presence)
base[:KeepAlive] = always
elsif @keep_alive.key?(:successful_exit)
base[:KeepAlive] = { SuccessfulExit: @keep_alive[:succesful_exit] }
elsif @keep_alive.key?(:crashed)
base[:KeepAlive] = { Crashed: @keep_alive[:crashed] }
elsif @keep_alive.key?(:path)
base[:KeepAlive] = { PathState: @keep_alive[:path].to_s }
end

which of these keys are you still going to set if they are either false or nil (separate questions)?

Copy link
Member Author

Choose a reason for hiding this comment

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

All except path for false, for nil I doubt there is a use.

Copy link
Member Author

Choose a reason for hiding this comment

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

Any remaining comments based on this?

Copy link
Member

Choose a reason for hiding this comment

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

Nah, find as-is.

Library/Homebrew/service.rb Outdated Show resolved Hide resolved
docs/Formula-Cookbook.md Outdated Show resolved Hide resolved
@SMillerDev SMillerDev force-pushed the feature/service/socket_and_keepalive branch from 8c68a67 to d8c30ad Compare April 11, 2022 18:59
@SMillerDev SMillerDev force-pushed the feature/service/socket_and_keepalive branch from d8c30ad to 3d5d12e Compare April 12, 2022 10:17
@SMillerDev SMillerDev merged commit 624ea2a into Homebrew:master Apr 14, 2022
@github-actions github-actions bot added the outdated PR was locked due to age label May 15, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 15, 2022
@SMillerDev SMillerDev deleted the feature/service/socket_and_keepalive branch September 21, 2022 13:19
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.

None yet

4 participants