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

fix the --filename option` #659

Merged
merged 1 commit into from
Dec 18, 2019
Merged

fix the --filename option` #659

merged 1 commit into from
Dec 18, 2019

Conversation

shivnagarajan
Copy link
Contributor

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.

What are you trying to accomplish with this PR?
...

How is this accomplished?
...

What could go wrong?
...

```$ 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.
@RyanBrushett
Copy link
Contributor

RyanBrushett commented Dec 17, 2019

@KnVerey
Copy link
Contributor

KnVerey commented Dec 18, 2019

Actually the design doc has it as singular, which is mirroring kubectl's arg:

 -f, --filename=[]: Filename, directory, or URL to files identifying the resource to get from a server.
 -f, --filename=[]: that contains the configuration to apply
 -f, --filename=[]: Filename, directory, or URL to files to use to create the resource

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?

@RyanBrushett
Copy link
Contributor

Yeah you're right, something to fix when we do the Thor bump.
As of today though, --filenames is what the render command actually takes as an arg tho so the docs should probably reflect that. Worth making note of fixing this as part of #658?

@shivnagarajan
Copy link
Contributor Author

Yeah you're right, something to fix when we do the Thor bump.
As of today though, --filenames is what the render command actually takes as an arg tho so the docs should probably reflect that. Worth making note of fixing this as part of #658?

How would a future fix handle the case for clients that already use --filenames (plural) instead of --filename

e.g. https://github.com/Shopify/shopify-core-mcrouter/pull/84, I could just use -f instead for now, but I am thinking of any one else that could already have versions that use --filenames

@RyanBrushett
Copy link
Contributor

Hmm indeed, cutting that over would be a breaking change. Maybe an alias could be an option?

@shivnagarajan
Copy link
Contributor Author

Should I merge this? or just close it?

Copy link
Contributor

@dturn dturn left a 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.

@dturn dturn merged commit db1734e into master Dec 18, 2019
@dturn dturn deleted the fix-up-filename-option branch December 18, 2019 16:21
@dturn dturn temporarily deployed to rubygems January 22, 2020 19:12 Inactive
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants