Skip to content

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.


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

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

4 participants