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 jxa commented Dec 15, 2017

Add support for --control-url

  • deprecate --control
  • add new option --control-url
  • Control CLI Bug Fix #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"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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
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

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

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

@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`
- puma#1416 introduced a bug in pumactl the puma
  cli does not respond to `control-cli` option. On puma it is called `control`
- this commit
  puma@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
@nijikon
Copy link

nijikon commented Apr 26, 2018

Can this be merged?

@nateberkopec nateberkopec merged commit 6d0efee into puma:master May 9, 2018
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.

None yet

4 participants