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

control_cli.rb - add require_relative 'log_writer' #3187

Merged
merged 4 commits into from
Jun 30, 2023

Conversation

MSP-Greg
Copy link
Member

Description

control_cli.rb may create Some Puma objects, dependent on the setup used (config, pid, or state file).

It may create a Puma::Configuration, which uses Puma::DSL, which may create a Puma::LogWriter, a Puma::LogWriteris also created when using the start method.

When used in a 'Puma' process, puma.rb autoloads Puma::LogWriter, but that file isn't required in control_cli.rb.

Closes #3186.

Your checklist for this pull request

  • I have reviewed the guidelines for contributing to this repository.
  • I have added (or updated) appropriate tests if this PR fixes a bug or adds a feature.
  • My pull request is 100 lines added/removed or less so that it can be easily reviewed.
  • If this PR doesn't need tests (docs change), I added [ci skip] to the title of the PR.
  • If this closes any issues, I have added "Closes #issue" to the PR description or my commit messages.
  • I have updated the documentation accordingly.
  • All new and existing tests passed, including Rubocop.

@dentarg
Copy link
Member

dentarg commented Jun 29, 2023

Is it too messy to test this? Feels like it could brake again in the future

@MSP-Greg
Copy link
Member Author

Is it too messy to test this?

Anything is possible, but, yes, it was a PITA

Updated.

Added test fails in master with:

  1) Failure:
TestIntegrationPumactl#test_require_dependencies [/mnt/c/Greg/GitHub/puma/test/test_integration_pumactl.rb:200]:
Expected "uninitialized constant Puma::DSL::LogWriter\n" to include "Command restart sent success".
  1. Added a test, but since the previous use of pumactl (TestIntegration#cli_pumactl) started it via Puma::ControlCLI.new, any files loaded in the test process might corrupt the test. So, added TestIntegration##cli_pumactl_spawn, which runs pumactl from a command line in a sub-process.

  2. Previously, Puma::CLI was lazy loaded. Changed Puma::Configuration and Puma::StateFile to also lazy load.

  3. Note that the test has two uses of pumactl, one using a config file, and one using a state file.

Lastly, GitHub Actions appears to be messed up (again). Same branch passed in my fork.

@MSP-Greg MSP-Greg added the bug label Jun 29, 2023
Copy link
Member

@dentarg dentarg left a comment

Choose a reason for hiding this comment

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

👍

@MSP-Greg
Copy link
Member Author

@dentarg

Thanks. Don't know if you looked at the 2nd run, and for those that aren't often looking at Actions job logs, Actions seems to intermittently have problems with external connections.

I've seen problems reaching keyservers, OS package sites (installing packages with apt, brew, etc), and even RubyGems. In the second run here, one of the JRuby jobs failed when installing ragel, see https://github.com/puma/puma/actions/runs/5414643664/jobs/9853016363?pr=3187#step:4:111

@MSP-Greg MSP-Greg merged commit 7c85305 into puma:master Jun 30, 2023
62 of 64 checks passed
@MSP-Greg MSP-Greg deleted the 00-issue-3186 branch June 30, 2023 12:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

"uninitialized constant Puma::DSL::LogWriter" on "pumactl restart"
2 participants