-
Notifications
You must be signed in to change notification settings - Fork 115
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
fix the --filename option` #659
Conversation
```$ krane help render Usage: krane render Options: [--bindings=foo=bar abc=def] # Bindings for erb f, [--filenames=config/deploy/production config/deploy/my-extra-resource.yml] # Directories and files to render [--stdin], [--no-stdin] # Read resources from stdin [--current-sha=SHA] # Expose SHA `current_sha` in ERB bindings Render templates ``` the `--filename` option needs to be `--filenames` to be consistent with the code/usage description.
Good eye Shiv. |
Actually the design doc has it as singular, which is mirroring kubectl's arg:
That makes more sense when the way to add multiple files is to repeat the arg, which we couldn't do because of the Thor bug... but that's fixed upsteam, right? |
Yeah you're right, something to fix when we do the Thor bump. |
How would a future fix handle the case for clients that already use e.g. https://github.com/Shopify/shopify-core-mcrouter/pull/84, I could just use |
Hmm indeed, cutting that over would be a breaking change. Maybe an alias could be an option? |
Should I merge this? or just close it? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Our docs should match reality, we can discuss adding an additional alias for --filename
in a new issue.
the
--filename
option needs to be--filenames
to be consistent with the code/usage description.What are you trying to accomplish with this PR?
...
How is this accomplished?
...
What could go wrong?
...