deis / workflow-cli

The CLI for Deis Workflow
http://deis.com
MIT License
31 stars 43 forks source link

fix(healthcheck): Parse arguments properly for httpheaders #280

Closed kmala closed 7 years ago

kmala commented 7 years ago

fixes #279

requires https://github.com/deis/controller-sdk-go/pull/110

deis-bot commented 7 years ago

@bacongobbler, @ultimateboy and @Joshua-Anderson are potential reviewers of this pull request based on my analysis of git blame information. Thanks @kmala!

bacongobbler commented 7 years ago

the ... in the docstring means that you can supply this argument multiple times, so the usage should be --header=X_DEIS_IS:awesome --header=X_MY_APP_IS_ALSO:awesome. Does that not work?

bacongobbler commented 7 years ago

From docopt's README:

... (ellipsis) one or more elements. To specify that arbitrary number of repeating elements could be accepted, use ellipsis (...), e.g. my_program.py FILE ... means one or more FILE-s are accepted. If you want to accept zero or more elements, use brackets, e.g.: my_program.py [FILE ...]. Ellipsis works as a unary operator on the expression to the left.

If your usage patterns allows to match same-named option with argument or positional argument several times, the matched arguments will be collected into a list:

Usage: my_program.py <file> <file> --path=<path>...

I.e. invoked with my_program.py file1 file2 --path=./here --path=./there the returned dict will contain args['<file>'] == ['file1', 'file2'] and args['--path'] == ['./here', './there'].

kmala commented 7 years ago

no they aren't working for the options but are working for the arguments.

bacongobbler commented 7 years ago

Any way we can fix this upstream in docopt.go instead and rev the version here? @mboersma is the core maintainer so I'm sure we could sneak him a cookie or something to motivate him ;)

mboersma commented 7 years ago

Love to see a PR patch for docopt-go! :+1:

codecov-io commented 7 years ago

Current coverage is 72.22% (diff: 100%)

Merging #280 into master will decrease coverage by 0.27%

@@             master       #280   diff @@
==========================================
  Files            59         59          
  Lines          4073       4176   +103   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits           2953       3016    +63   
- Misses          795        820    +25   
- Partials        325        340    +15   

Powered by Codecov. Last update eb21ef2...2b7700d

kmala commented 7 years ago

i don't think we can have more than one arguments with ellipses(...) because the values are separated by space which means it can't differentiate mapping between the values and the args if there are more

bacongobbler commented 7 years ago

Sounds good. This LGTM with the controller-sdk bump with the fix as well. 👍