Skip to content

Add brew bundle exec --services #19552

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

Merged
merged 7 commits into from
Mar 28, 2025
Merged

Add brew bundle exec --services #19552

merged 7 commits into from
Mar 28, 2025

Conversation

Bo98
Copy link
Member

@Bo98 Bo98 commented Mar 20, 2025

A nice shortcut to quickly start and stop all formulae with services in your Brewfile. This makes it nicer if you have e.g. differing database versions (which often still share the same port) between projects.

@Bo98 Bo98 force-pushed the bundle-services branch from 4b126ef to a4a04de Compare March 20, 2025 07:41
MikeMcQuaid
MikeMcQuaid previously approved these changes Mar 20, 2025
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.

Thanks, looks good so far! What happens if there's an existing service already running, will this automatically stop it? If not: that seems desirable.

@Bo98
Copy link
Member Author

Bo98 commented Mar 21, 2025

What happens if there's an existing service already running, will this automatically stop it?

Not currently. brew services does not support temporarily stopping a service well at the moment (there's kill but the service can just auto-restart). I want to avoid removing the LaunchAgents service.

Not sure how such CLI should look. brew services stop --keep?

@Bo98 Bo98 force-pushed the bundle-services branch 2 times, most recently from d300326 to 486ec7d Compare March 21, 2025 07:24
@MikeMcQuaid
Copy link
Member

Not currently. brew services does not support temporarily stopping a service well at the moment (there's kill but the service can just auto-restart). I want to avoid removing the LaunchAgents service.

Not sure how such CLI should look. brew services stop --keep?

Don't feel strongly about the API. I agree with not removing the service if possible but, if it's not, it feels more sensible to remove it.

I think for run to be useful in this context, it needs to be able to also stop. If one project is running postgresql@14 and another postgresql@15 on the same ports, you need to be able to provide a way to ensure that brew bundle services can not only start postgresql@15 but also stop postgresql@14 if needed. We already have some context for this with the conflicts_with: ["mysql"] brew bundle DSL; this feels like another good use of that.

@Bo98 Bo98 force-pushed the bundle-services branch from 486ec7d to 81856ea Compare March 25, 2025 03:44
@Bo98 Bo98 changed the base branch from master to bundle_version_file March 25, 2025 03:44
@Bo98
Copy link
Member Author

Bo98 commented Mar 25, 2025

Rebased on #19579 to avoid conflicts.

carlocab
carlocab previously approved these changes Mar 25, 2025
@Bo98 Bo98 force-pushed the bundle-services branch from 81856ea to 0492fa1 Compare March 25, 2025 03:55
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.

Thanks @Bo98! Not sure exactly how but it feels like the e.g. service_change_state! logic in bundle/brew_installer.rb and/or bundle/brew_services.rb should be integrated here. Not sure I understand why/when you'd use bundle/brew_services.rb vs. bundle/services?

Base automatically changed from bundle_version_file to master March 25, 2025 10:25
@MikeMcQuaid MikeMcQuaid dismissed stale reviews from carlocab and themself March 25, 2025 10:25

The base branch was changed.

@Bo98 Bo98 force-pushed the bundle-services branch from 0492fa1 to a6f9f6c Compare March 27, 2025 06:12
@Bo98
Copy link
Member Author

Bo98 commented Mar 27, 2025

it feels like the e.g. service_change_state! logic in bundle/brew_installer.rb

It intentionally behaves a little differently but indeed this should at least be following the versioned env path so I've made it now do so.

bundle/brew_services.rb should be integrated here

I've done this now

Not sure I understand why/when you'd use bundle/brew_services.rb vs. bundle/services?

I've folded the latter into bundle/commands/services to avoid confusion.

@Bo98 Bo98 force-pushed the bundle-services branch 2 times, most recently from 1f5e3d9 to f0f9bcf Compare March 27, 2025 06:31
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.

Looking good! Might be nice to extract the brew bundle services commands into another PR; think they require a bit more thought/discussion and the rest seems almost ready to go.

@Bo98 Bo98 force-pushed the bundle-services branch from f0f9bcf to c273d8b Compare March 28, 2025 05:55
@Bo98 Bo98 changed the title Add brew bundle services helper Add brew bundle exec --services Mar 28, 2025
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.

Thanks @Bo98, great work! Merging as-is but would love some of the missing test coverage to come in a follow-up PR.

exit_code = 0
run_services(@dsl.entries) do
Kernel.system(*args)
exit_code = $CHILD_STATUS.exitstatus
Copy link
Member

Choose a reason for hiding this comment

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

Would love a follow-up PR with some test coverage here.

).void
}
private_class_method def self.map_service_info(entries, &_block)
entries_formulae = entries.filter_map do |entry|
Copy link
Member

Choose a reason for hiding this comment

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

Would love a follow-up PR with some test coverage here.


sig { params(entries: T::Array[Homebrew::Bundle::Dsl::Entry], _block: T.nilable(T.proc.void)).void }
private_class_method def self.run_services(entries, &_block)
services_to_restart = []
Copy link
Member

Choose a reason for hiding this comment

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

Would love a follow-up PR with some test coverage here.


sig { params(entries: T::Array[Homebrew::Bundle::Dsl::Entry]).void }
private_class_method def self.stop_services(entries)
map_service_info(entries) do |info, _, _|
Copy link
Member

Choose a reason for hiding this comment

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

Would love a follow-up PR with some test coverage here.

@MikeMcQuaid MikeMcQuaid added this pull request to the merge queue Mar 28, 2025
Merged via the queue into master with commit 2603401 Mar 28, 2025
36 checks passed
@MikeMcQuaid MikeMcQuaid deleted the bundle-services branch March 28, 2025 09:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants