Add Live Reload #37

joshuatcasey · 2022-07-18T16:02:08Z

Add live reload

Resolves #33

Checklist

I have viewed, signed, and submitted the Contributor License Agreement.
I have linked issue(s) that this PR should close using keywords or the Github UI (See docs)
I have added an integration test, if necessary.
I have reviewed the styleguide for guidance on my code quality.
I'm happy with the commit history on this PR (I have rebased/squashed as needed).

joshuatcasey · 2022-07-18T16:09:30Z

~~Blocked on a release of https://github.com/paketo-buildpacks/libreload-packit~~

This PR no longer needs libreload-packit.

README.md

integration/httpd_reload_test.go

sophiewigmore · 2022-07-21T14:32:51Z

The changes here look reasonable to me, but I'd like to be able to test it out locally when a release is available of libreload-packit

joshuatcasey · 2022-07-21T14:50:22Z

The changes here look reasonable to me, but I'd like to be able to test it out locally when a release is available of libreload-packit

Sounds good. It can be tested locally with the prerelease version of libreload-packit which is why this PR isn't in Draft but of course we need to wait.

Another thought I had - do we have confidence that we're not leaking processes by reloading procmgr-binary? I'll set some time aside (probably tomorrow) to manually inspect the process list.

sophiewigmore · 2022-07-22T13:51:41Z

@joshuatcasey what do you mean by leaking processes?

joshuatcasey · 2022-07-22T16:56:18Z

@joshuatcasey what do you mean by leaking processes?

procmgr-binary kicks off fpm and nginx|httpd. If watchexec restarts procmgr-binary, I'm curious if those fpm and nginx|httpd processes are cleaned up correctly and new processes created.

joshuatcasey · 2022-07-22T19:25:45Z

@sophiewigmore while investigating the "leaking processes" above, I had a brainwave: do we really even need watchexec here? Won't httpd and nginx just pick up file changes?

And it looks like that answer is.... Yes. I'll remove watchexec and libreload-packit from this PR shortly.

joshuatcasey · 2022-07-22T19:52:52Z

I've force-pushed this branch so that it only contains README and integration test changes.

The original "working" PR is over on branch jtc/add-watchexec but we don't need to save that once this PR is merged.

sophiewigmore · 2022-07-26T14:42:23Z

integration/nginx_reload_test.go

+		docker = occam.NewDocker()
+	})
+
+	context("when BP_LIVE_RELOAD_ENABLED is true", func() {


@joshuatcasey I'm confused about this. BP_LIVE_RELOAD_ENABLED is not set to true in the test - does this line need to be modified? Or should BP_LIVE_RELOAD_ENABLED be set?

I am assuming just this line needs to change?

@joshuatcasey this still needs to change since the env var isn't set.

sophiewigmore · 2022-07-26T15:18:48Z

@TisVictress why was the BP_LIVE_RELOAD_ENABLED added to the HTTPD/Nginx buildpacks if that functionality works without the watchexec buildpack?

TisVictress · 2022-07-26T15:27:30Z

@TisVictress why was the BP_LIVE_RELOAD_ENABLED added to the HTTPD/Nginx buildpacks if that functionality works without the watchexec buildpack?

I needed to make this change in the nginx and httpd buildpacks so that I could add support for watchexec to the web-servers buildpack

sophiewigmore · 2022-07-26T15:31:42Z

@TisVictress but my question is why is that buildpack needed if this functionality works without watchexec?

sophiewigmore · 2022-07-26T21:35:50Z

Current state of this PR:

I'd like to understand if the php-fpm start command part of this buildpack need to be reloaded. In the current implementation, this is not a reloadable process type as far as I know.
Even though as seen in this PR, watchexec isn't needed, should we include it for parity with what the Nginx and HTTPD buildpacks do? It seems silly to pull in the watchexec buildpack for no reason. Does this implementation cover the same use cases as using the watchexec buildpack?

sophiewigmore · 2022-10-13T13:06:07Z

@TisVictress this PR is still in progress!

joshuatcasey · 2022-10-17T23:41:35Z

I think it looks good as of 8d026a0c74b375d4dfed5331cf0329980eac152d!

joshuatcasey · 2022-10-19T12:21:24Z

@paketo-buildpacks/php-maintainers Are we good to merge this?

sophiewigmore · 2022-10-19T19:29:19Z

I'd love to get an additional review from @thitch97 on this before we merge since you and I Josh have been collaborating on this a bit. The changes look good to me, though! :)

…not exist

…est logging

joshuatcasey requested a review from a team as a code owner July 18, 2022 16:02

joshuatcasey requested review from sophiewigmore and paketo-bot-reviewer July 18, 2022 16:02

joshuatcasey added semver:minor A change requiring a minor version bump status/blocked This issue has been triaged and resolving it is blocked on some other issue labels Jul 18, 2022

joshuatcasey force-pushed the jtc/issue-33 branch from a980107 to ae96c06 Compare July 18, 2022 20:44

joshuatcasey mentioned this pull request Jul 19, 2022

Use live reload library paketo-buildpacks/poetry-run#47

Closed

sophiewigmore reviewed Jul 21, 2022

View reviewed changes

README.md Show resolved Hide resolved

sophiewigmore reviewed Jul 21, 2022

View reviewed changes

integration/httpd_reload_test.go Outdated Show resolved Hide resolved

joshuatcasey force-pushed the jtc/issue-33 branch from ae96c06 to 1e65fec Compare July 21, 2022 14:40

joshuatcasey force-pushed the jtc/issue-33 branch from 8afefef to db84bf2 Compare July 22, 2022 16:53

joshuatcasey marked this pull request as draft July 22, 2022 19:26

joshuatcasey force-pushed the jtc/issue-33 branch from db84bf2 to 5e8e6cb Compare July 22, 2022 19:51

joshuatcasey marked this pull request as ready for review July 22, 2022 19:52

joshuatcasey removed the status/blocked This issue has been triaged and resolving it is blocked on some other issue label Jul 25, 2022

TisVictress previously approved these changes Jul 26, 2022

View reviewed changes

sophiewigmore reviewed Jul 26, 2022

View reviewed changes

joshuatcasey added semver:patch A change requiring a patch version bump and removed semver:minor A change requiring a minor version bump labels Jul 26, 2022

joshuatcasey added semver:minor A change requiring a minor version bump and removed status/blocked This issue has been triaged and resolving it is blocked on some other issue semver:patch A change requiring a patch version bump labels Oct 7, 2022

joshuatcasey force-pushed the jtc/issue-33 branch from 6d3f5eb to 7cc40ef Compare October 12, 2022 19:46

TisVictress previously approved these changes Oct 13, 2022

View reviewed changes

sophiewigmore dismissed TisVictress’s stale review via c7a3360 October 17, 2022 20:44

joshuatcasey force-pushed the jtc/issue-33 branch from c7a3360 to 8d026a0 Compare October 17, 2022 23:41

sophiewigmore previously approved these changes Oct 19, 2022

View reviewed changes

thitch97 previously approved these changes Oct 27, 2022

View reviewed changes

joshuatcasey and others added 11 commits October 27, 2022 10:02

Advertise live reload capability in the README and integration tests

80042c0

Add live reload for nginx and httpd conf files

f763fed

Remove extra code and comments

21b40d6

Simplify process definition

b5053a0

Log at DEBUG when live reload is requested but the config folders do …

6456d5f

…not exist

Use Serve(WithHeader) from occam

f0b69ae

Fix commented test code

945cceb

Check in empty files so that the directories exist, for integration t…

889c37c

…est logging

Use --shell=none with watchexec

efc24a5

Use SIGUSR2 to reload FPM configuration

fe9231a

add FPM reload test

114a843

joshuatcasey dismissed stale reviews from thitch97 and sophiewigmore via 114a843 October 27, 2022 15:03

joshuatcasey force-pushed the jtc/issue-33 branch from a2419d6 to 114a843 Compare October 27, 2022 15:03

thitch97 approved these changes Oct 27, 2022

View reviewed changes

joshuatcasey merged commit 88635e4 into main Oct 27, 2022

joshuatcasey deleted the jtc/issue-33 branch October 27, 2022 15:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Live Reload #37

Add Live Reload #37

joshuatcasey commented Jul 18, 2022

joshuatcasey commented Jul 18, 2022 •

edited

Loading

sophiewigmore commented Jul 21, 2022

joshuatcasey commented Jul 21, 2022

sophiewigmore commented Jul 22, 2022

joshuatcasey commented Jul 22, 2022

joshuatcasey commented Jul 22, 2022

joshuatcasey commented Jul 22, 2022

sophiewigmore Jul 26, 2022 •

edited

Loading

sophiewigmore Sep 9, 2022

sophiewigmore commented Jul 26, 2022

TisVictress commented Jul 26, 2022 •

edited

Loading

sophiewigmore commented Jul 26, 2022

sophiewigmore commented Jul 26, 2022 •

edited

Loading

sophiewigmore commented Oct 13, 2022

joshuatcasey commented Oct 17, 2022

joshuatcasey commented Oct 19, 2022

sophiewigmore commented Oct 19, 2022

Add Live Reload #37

Add Live Reload #37

Conversation

joshuatcasey commented Jul 18, 2022

Checklist

joshuatcasey commented Jul 18, 2022 • edited Loading

sophiewigmore commented Jul 21, 2022

joshuatcasey commented Jul 21, 2022

sophiewigmore commented Jul 22, 2022

joshuatcasey commented Jul 22, 2022

joshuatcasey commented Jul 22, 2022

joshuatcasey commented Jul 22, 2022

sophiewigmore Jul 26, 2022 • edited Loading

Choose a reason for hiding this comment

sophiewigmore Sep 9, 2022

Choose a reason for hiding this comment

sophiewigmore commented Jul 26, 2022

TisVictress commented Jul 26, 2022 • edited Loading

sophiewigmore commented Jul 26, 2022

sophiewigmore commented Jul 26, 2022 • edited Loading

sophiewigmore commented Oct 13, 2022

joshuatcasey commented Oct 17, 2022

joshuatcasey commented Oct 19, 2022

sophiewigmore commented Oct 19, 2022

joshuatcasey commented Jul 18, 2022 •

edited

Loading

sophiewigmore Jul 26, 2022 •

edited

Loading

TisVictress commented Jul 26, 2022 •

edited

Loading

sophiewigmore commented Jul 26, 2022 •

edited

Loading