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

Use symfony/yaml, new Soketi service, and new sail:add command #532

Merged
merged 13 commits into from
Feb 1, 2023

Conversation

tonysm
Copy link
Contributor

@tonysm tonysm commented Jan 27, 2023

Changed

  • Adds the symfony/yaml dependency to ease Yaml manipulation (there are some differences to the generated Yaml file, I'll try to describe them below)
  • Extracted some of the InstallCommand methods into a InteractsWithDockerComposeServices concern (trait). I needed those methods in the new sail:add command.

Add

  • Adds a new artisan sail:add command that we can use to add new services to our Sail setup. We're currently expecting this command to run from the host machine (since it uses some docker commands in the prepareInstallation method). I think the sail:install command also has the same expectation (I've always ran it from my host machine)
  • Adds a new soketi service to ease broadcasting setup on Sail

There are some changes to the Yaml generated by the symfony/yaml package, especially around sequences. One example is the Redis health-check command:

Before:

services:
    # ...
    redis:
        # ...
        healthcheck:
            test: ["CMD", "redis-cli", "ping"]

After:

services:
    # ...
    redis:
        # ...
        healthcheck:
            test:
                - CMD
                - redis-cli
                - ping

Both versions work the same, I guess it's just aesthetics (I prefer the before). Couldn't find a way to change it (maybe we could do some string manipulation as before?)

I explored adding a sail add command to the bin/sail script, but not sure. The script would need a few changes, 'cause we can run php artisan sail:add inside the container but we'd need to run the docker commands from the host machine (unless we share the docker socket with the container and install docker in the laravel.test, this way I believe we could run the prepareInstallation method inside the container as well).

Why not use the artisan sail:install to add the new dependencies?

That's what I've been using. But I always had to list all services I wanted to keep + the new service. Even though the new sail:add command and the sail:install command are technically the same now (after the changes here), the add one passes the intention of addition. But I guess it's not necessary.


This is all a proof of concept. Wasn't sure where to suggest the sail:add feature, but the GH workflow said to open a PR. Happy to open separate PRs in case y'all don't want to add an extra dependency, or don't want to add soketi, etc.

@tonysm tonysm changed the title Uses symfony/yaml, new Soketi service, and new sail:add command Use symfony/yaml, new Soketi service, and new sail:add command Jan 27, 2023
@taylorotwell
Copy link
Member

taylorotwell commented Jan 31, 2023

Hey @tonysm - this PR has some conflicts. Please mark as ready for review when those are resolved.

@taylorotwell taylorotwell marked this pull request as draft January 31, 2023 21:24
@tonysm tonysm marked this pull request as ready for review February 1, 2023 01:58
@tonysm
Copy link
Contributor Author

tonysm commented Feb 1, 2023

@taylorotwell I've rebased it. By the way, I only opened this as a discussion starter. No pressure to merge it. Happy to open separate PRs for each of the changes I described here or to pick only the wanted pieces 👍🏼

@taylorotwell
Copy link
Member

@tonysm this all looks pretty good to me. I'm ready to merge it in if you are.

@tonysm
Copy link
Contributor Author

tonysm commented Feb 1, 2023

Alright 👍

@taylorotwell taylorotwell merged commit 6f19af8 into laravel:1.x Feb 1, 2023
@driesvints
Copy link
Member

Thanks @tonysm!

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.

3 participants