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

[1.x] Allow easy customization of the command ran by supervisor's PHP process #645

Merged
merged 3 commits into from
Dec 29, 2023

Conversation

bram-pkg
Copy link
Contributor

@bram-pkg bram-pkg commented Dec 28, 2023

This PR adds the capability to control supervisor's PHP process command using an environment variable, potentially simplifying the installation of Laravel Octane into dev envs using Laravel Sail.

Allowing for customization of the command ran by supervisor through an environment variable removes the need to publish all the underlying files used to build the image, in the simple use case of running Octane under Sail.

This does not introduce a breaking change, luckily, as changes to both the supervisord.conf and Dockerfile only apply after a rebuild, hence not affecting current users. After rebuilding (optional for current users), nothing would break either, as the environment variable has a sane default value that is equal to the current behaviour.

@taylorotwell
Copy link
Member

In the Supervisor configuration, why are the variables prefixed with ENV_SUPERVISOR_? I can find no reference to this in the Supervisor documentation, only prefixing with ENV_.

@taylorotwell taylorotwell marked this pull request as draft December 28, 2023 16:58
@taylorotwell
Copy link
Member

Mark as ready for review when answered please.

@bram-pkg
Copy link
Contributor Author

bram-pkg commented Dec 28, 2023

Oops, that was not meant to be like that. I've amended the variables in the Dockerfile files

@bram-pkg bram-pkg marked this pull request as ready for review December 28, 2023 17:08
@taylorotwell
Copy link
Member

So, that leads me to my next question: was this tested at all? 😅

@bram-pkg
Copy link
Contributor Author

bram-pkg commented Dec 28, 2023

Yes, I've got this running locally. Reason it was renamed was that I published the docker files in one of my projects to test this out, but decided to make the env var name more descriptive 😅 Simply forgot to include it in all places when committing to the fork.

It's working in my project now, together with the env var overridden in the docker-compose.yaml to run the Laravel Octane command rather than artisan serve

@bram-pkg
Copy link
Contributor Author

bram-pkg commented Dec 29, 2023

So for context, @taylorotwell, I have this in my docker-compose.yaml:

environment:
  # other env vars
  SUPERVISOR_PHP_COMMAND: "/usr/bin/php -d variablescommand=/usr/bin/php -d variables_order=EGPCS /var/www/html/artisan octane:frankenphp --host=0.0.0.0 --admin-port=2019 --port=80"

Which is working just fine :)

@taylorotwell taylorotwell merged commit 9fa62a7 into laravel:1.x Dec 29, 2023
3 checks passed
@taylorotwell
Copy link
Member

Can you send in a documentation PR to the Octane docs for this?

@bram-pkg
Copy link
Contributor Author

Yep, can do.

Do you want it to mention "Since 1.27.0"? @taylorotwell

@bram-pkg bram-pkg deleted the add-supervisor-env-var branch December 29, 2023 22:08
renovate bot added a commit to RadioRoster/backend that referenced this pull request Feb 9, 2024
[![Mend
Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com)

This PR contains the following updates:

| Package | Change | Age | Adoption | Passing | Confidence |
|---|---|---|---|---|---|
| [laravel/sail](https://togithub.com/laravel/sail) | `1.26.3` ->
`1.27.3` |
[![age](https://developer.mend.io/api/mc/badges/age/packagist/laravel%2fsail/1.27.3?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![adoption](https://developer.mend.io/api/mc/badges/adoption/packagist/laravel%2fsail/1.27.3?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![passing](https://developer.mend.io/api/mc/badges/compatibility/packagist/laravel%2fsail/1.26.3/1.27.3?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![confidence](https://developer.mend.io/api/mc/badges/confidence/packagist/laravel%2fsail/1.26.3/1.27.3?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|

---

### Release Notes

<details>
<summary>laravel/sail (laravel/sail)</summary>

###
[`v1.27.3`](https://togithub.com/laravel/sail/blob/HEAD/CHANGELOG.md#v1273---2024-01-30)

[Compare
Source](https://togithub.com/laravel/sail/compare/v1.27.2...v1.27.3)

- \[1.x] Improves console output by
[@&#8203;nunomaduro](https://togithub.com/nunomaduro) in
[laravel/sail#661

###
[`v1.27.2`](https://togithub.com/laravel/sail/blob/HEAD/CHANGELOG.md#v1272---2024-01-21)

[Compare
Source](https://togithub.com/laravel/sail/compare/v1.27.1...v1.27.2)

- Add Support for Typesense by
[@&#8203;jasonbosco](https://togithub.com/jasonbosco) in
[laravel/sail#655
- Lint sail script by
[@&#8203;dimitriacosta](https://togithub.com/dimitriacosta) in
[laravel/sail#656
- Make DB_CONNECTION replacement more robust by
[@&#8203;taylorotwell](https://togithub.com/taylorotwell) in
laravel/sail@2276a8d

###
[`v1.27.1`](https://togithub.com/laravel/sail/blob/HEAD/CHANGELOG.md#v1271---2024-01-13)

[Compare
Source](https://togithub.com/laravel/sail/compare/v1.27.0...v1.27.1)

- \[1.x] \[[#&#8203;651](https://togithub.com/laravel/sail/issues/651)]
Don't do anything if no phpunit files are present by
[@&#8203;zack6849](https://togithub.com/zack6849) in
[laravel/sail#652

###
[`v1.27.0`](https://togithub.com/laravel/sail/blob/HEAD/CHANGELOG.md#v1270---2024-01-03)

[Compare
Source](https://togithub.com/laravel/sail/compare/v1.26.3...v1.27.0)

- \[1.x] Allow easy customization of the command ran by supervisor's PHP
process by [@&#8203;bram-pkg](https://togithub.com/bram-pkg) in
[laravel/sail#645
- \[1.x] Default to PHP 8.3 by
[@&#8203;Jubeki](https://togithub.com/Jubeki) in
[laravel/sail#647

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined),
Automerge - At any time (no schedule defined).

🚦 **Automerge**: Enabled.

♻ **Rebasing**: Whenever PR is behind base branch, or you tick the
rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update
again.

---

- [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check
this box

---

This PR has been generated by [Mend
Renovate](https://www.mend.io/free-developer-tools/renovate/). View
repository job log
[here](https://developer.mend.io/github/RadioRoster/backend).

<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNy4xMjcuMCIsInVwZGF0ZWRJblZlciI6IjM3LjE1My4yIiwidGFyZ2V0QnJhbmNoIjoibWFpbiJ9-->

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
@RahulDey12
Copy link

@taylorotwell @bram-pkg Hey guys, these changes work but I recommend not to use this instruction with octane because octane runs like a demon which doesn't load the new code changes to the memory because of this if we change something it doesn't affect the application. Technically we can still use this with --watch param but watching file changes depends on the chokidar plugin which gives extra dependency to the application.

@bram-pkg
Copy link
Contributor Author

The behaviour of the daemon was not changed in this PR, adding the --watch parameter was not part of the original documentation for this customization either.

Adding the --watch parameter is documented here, and you can add that to the environment variable set as part of this PR.
https://laravel.com/docs/10.x/octane#watching-for-file-changes

@RahulDey12
Copy link

Yeah but if someone add the

SUPERVISOR_PHP_COMMAND: "/usr/bin/php -d variablescommand=/usr/bin/php -d variables_order=EGPCS /var/www/html/artisan octane:frankenphp --host=0.0.0.0 --admin-port=2019 --port=80"

to their code and start working as usual they will find that any changes they are doing is not affecting to the browser then they will discover that it happened because of the octane start without watching the file changes

@bram-pkg
Copy link
Contributor Author

Yes, which is why there is a section in the documentation about "watching for file changes"

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.

None yet

3 participants