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

Add support for --control-url in cli #1487

Merged
merged 1 commit into from May 9, 2018
Merged

Conversation

@jxa
Copy link
Contributor

jxa commented Dec 15, 2017

Add support for --control-url

  • deprecate --control
  • add new option --control-url
  • #1416 introduced a bug in pumactl the puma cli does not respond to control-cli option. On puma it is called control
  • this commit a8f54f7 was correct, but it looked like a typo since the names of the options are not consistent across versions
  • the best option for fixing this seems to be to support both options in the cli
  • we don't want to stop supporting control in the cli for backwards-compatibility
@jxa
Copy link
Contributor Author

jxa commented Dec 15, 2017

Also reported here #1477

@jxa
Copy link
Contributor Author

jxa commented Dec 15, 2017

an alternative approach is to simply revert #1416, and write a spec for pumactl. downside being that the options do not match across both interfaces.

@jxa jxa changed the title Add alias support for both `control` and `control-url` options in cli Add support for --control-url in cli Dec 15, 2017
@jlduran
Copy link

jlduran commented Dec 15, 2017

The README.md also references --control. But that can be easily addressed in a separate commit.

@@ -220,7 +220,7 @@ def send_signal
end

def run
start if @command == "start"
return start if @command == "start"

This comment has been minimized.

Copy link
@jxa

jxa Dec 19, 2017

Author Contributor

Prior to this change, send_request was being called with @command == "start". The control server does not respond to this command, so returns a 404, which then ultimately calls exit 1, making it very difficult to figure out why my test was failing silently. This early return short-circuits all that mess.

@jxa
Copy link
Contributor Author

jxa commented Dec 19, 2017

I'm open to options on how to actually fix the CLI @evanphx @nateberkopec @schneems.

Whatever the ultimate decision, I think this is an extremely valuable test which will prevent similar accidental breakage in the future.

@jxa
Copy link
Contributor Author

jxa commented Dec 19, 2017

/sigh that is, it will be valuable if I can get it to pass reliably on CI

@jxa jxa force-pushed the jxa:control-url branch from bdcd8a1 to ae6548b Jan 7, 2018
@jxa
Copy link
Contributor Author

jxa commented Jan 7, 2018

Travis build is failing for ruby 2.1, but not just on this PR. These other PRs have the same failure:

@nateberkopec
Copy link
Member

nateberkopec commented Mar 19, 2018

@jxa Sorry for the delay here, I've been taking a break from Ruby.

Can you rebase? I think we fixed the build issues.

@nateberkopec nateberkopec added the bug label Mar 19, 2018
@jxa jxa force-pushed the jxa:control-url branch from ae6548b to ccbe686 Apr 12, 2018
@jxa
Copy link
Contributor Author

jxa commented Apr 12, 2018

@nateberkopec sorry I missed your mention somehow! I have rebased, but the appveyor build has failed on 2 versions while trying to compile with ragel. I doubt these are related to my changes. Any advice on how to deal with this issue?

- deprecate `--control`
- add new option `--control-url`
- #1416 introduced a bug in pumactl the puma
  cli does not respond to `control-cli` option. On puma it is called `control`
- this commit
  a8f54f7
  was correct, but it looked like a typo since the names of the options are not
  consistent across versions
- the best option for fixing this seems to be to support both options
- we don't want to stop supporting `control` in the cli for
  backwards-compatibility so it is deprecated
@jxa jxa force-pushed the jxa:control-url branch from ccbe686 to cbe9329 Apr 13, 2018
@nijikon
Copy link

nijikon commented Apr 26, 2018

Can this be merged?

@nateberkopec nateberkopec merged commit 6d0efee into puma:master May 9, 2018
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.