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

Knife command-line refactor plus addl features #69

Closed
wants to merge 9 commits into from

Conversation

eklein
Copy link
Contributor

@eklein eklein commented Jan 4, 2014

  • Refactored all knife commands to use the syntax:

    knife vault <sub-command>

  • Modified knife vault create to allow popping an editor window when values, json_file or file are unspecified on the command-line.

  • Added knife vault edit.

  • Changed all the banners to say:

    knife vault <sub-command> VAULT ITEM VALUES (options)

    Instead of listing all the options out. This is more consistent with most other knife plugins I've used.

Please let me know what you think. I know it's a lot, but I've been thinking about tackling this for a while and finally got motivated last night/this morning :)

Happy to revise my commits or squash if it's easier as a single commit.

@moserke
Copy link
Contributor

moserke commented Jan 4, 2014

Thanks for these changes. I kind of like the approach of doing a minor version bump but having the old commands very clearly state they are deprecated and will be removed in the next major version bump. This gives people a bit of heads up and then dump them in the next major version release. But definitely if is released without them it would need to be a major version bump. (I'll probably take care of supporting these before I package back up, no need to make you have to rework everything) When I work on merging yours and @jgeiger pull requests I might need to have you resolve some merge conflicts since you worked on the same thing.

Won't be able to get to these probably until Monday, want to take some time to pull the branch down and look it over good, mostly because the specs are pretty slim.

Beefing up the specs is the next thing I really want to focus on for the gem because as it gets more contributors and work done on it, it's going to be really important that it's tested well.

Thanks for your work so far Eli, love having the help.

@eklein
Copy link
Contributor Author

eklein commented Jan 5, 2014

I can revise these commits to include the old knife commands with deprecation warnings if that would help. It would be pretty trivial since the new commands are in completely separate files. Do you mind having the documentation updated such that only the new commands are documented or do you want the old stuff in there as well?

@eklein
Copy link
Contributor Author

eklein commented Jan 5, 2014

Also, agreed on the specs. Happy to help out on that front as well.

@eklein
Copy link
Contributor Author

eklein commented Jan 14, 2014

this has replaced by PR #72

@eklein eklein closed this Jan 14, 2014
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.

2 participants