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

Puma::ControlCLI - allow refork command to be sent as a request #2868

Merged
merged 3 commits into from
Jun 2, 2022

Conversation

MSP-Greg
Copy link
Member

@MSP-Greg MSP-Greg commented Apr 29, 2022

Description

Currently, the refork command will only be sent as a signal by Puma::ControlCLI. Change to allow it be sent as a request if the control server is setup with a uri.

Closes #2866

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.

@@ -33,10 +33,10 @@ class ControlCLI
COMMANDS = CMD_PATH_SIG_MAP.keys.freeze

# commands that cannot be used in a request
NO_REQ_COMMANDS = %w{refork}.freeze
NO_REQ_COMMANDS = %w[].freeze
Copy link
Member

Choose a reason for hiding this comment

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

Probably should just remove this constant then, yes?

Copy link
Member

Choose a reason for hiding this comment

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

I feel like I'm missing something.

Copy link
Member Author

Choose a reason for hiding this comment

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

I can, but I thought I'd leave the code in case a 'signal only' command was added. Or, previous to this PR, refork was a signal only command, and I never tested it using a control server that had a control uri.

Leaving as is means that CI will check that if one is added. Ok, I know it's a little odd...

Copy link
Member Author

@MSP-Greg MSP-Greg May 20, 2022

Choose a reason for hiding this comment

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

I commented out the constant and the clause that used it. Same for the test (skipped if the constant is undefined).

I can remove if you want... I just don't want myself or someone else making the same mistake again, so the commented code leaves a big hint...

# commands that cannot be used in a request
NO_REQ_COMMANDS = %w{refork}.freeze
# NO_REQ_COMMANDS = %w[].freeze
Copy link
Contributor

Choose a reason for hiding this comment

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

Should just remove this altogether?

@nateberkopec nateberkopec added the waiting-for-review Waiting on review from anyone label May 24, 2022
@MSP-Greg
Copy link
Member Author

I rebased and added 'signal only' commands that were previously unavailable using ControlCLI. See eb87fe5

I think I found all of them. Hence, ControlCLI can be used to send all signal commands that work with the main Puma process.

@nateberkopec nateberkopec merged commit 6543f09 into puma:master Jun 2, 2022
@MSP-Greg MSP-Greg deleted the 00-2866-refork-2 branch June 2, 2022 20:50
nateberkopec pushed a commit that referenced this pull request Aug 22, 2022
* Puma::ControlCLI - allow refork command to be sent as a request

* Puma::ControlCLI - add signal only commands

* Puma::ControlCLI - check whether signal is available
JuanitoFatas pushed a commit to JuanitoFatas/puma that referenced this pull request Sep 9, 2022
…#2868)

* Puma::ControlCLI - allow refork command to be sent as a request

* Puma::ControlCLI - add signal only commands

* Puma::ControlCLI - check whether signal is available
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug feature waiting-for-review Waiting on review from anyone
Projects
None yet
Development

Successfully merging this pull request may close these issues.

"Invalid request command: pumactl refork"
3 participants