Krane CLI

[KnVerey]

We've decided to try Thor for this (see discussion on this issue). The new CLI must:

• be easy to test (and well-tested)
• enforce required args (addressing Argument parsing in scripts is too permissive #482)
• implement the changes to the command and flags detailed in our Design doc

[ramontayag]

Add this to the list too https://github.com/commander-rb/commander

[timothysmith0609]

I think the fact that Thor uses open-uri might be enough to disqualify it since it allows for remote code execution.
CVE report
Interesting reading

[KnVerey]

Interesting find, Tim. The maintainers (which apparently include Rafael) argue the CVE isn't actually relevant to a CLI tool: Thor issue. Apparently it only affects Thor's get command, which we wouldn't have a use for.

[benlangfeld]

@KnVerey Has a library on which to build this been chosen? I would be happy to prep a PR for this if so.

[KnVerey]

We haven't chosen yet, but I'll bring it up again with my team and try to get back to you soon.

[KnVerey]

I think Thor makes the most sense for this project. However, please note that we have a few other changes that we'd to take the opportunity to make in this transition, beyond just splitting things like kubernetes-deploy -> krane deploy. Unfortunately these aren't documented in a single place yet. I've added a work item to our internal tracker to produce a proper design document for this.

[benlangfeld]

@KnVerey I have a little time available to deal with this. Is it worth me making a start? Do you have any references to those other changes you'd like to make?

[KnVerey]

Hey @benlangfeld thanks for letting me know you'd like to start this soon. I started a draft of the design doc and will post it here for comment by the end of the week.

[KnVerey]

Here is the proposed design as promised @benlangfeld : https://docs.google.com/document/d/1oInUsKplYGNWTymPY48xtDCx3x4Cwc-phcEKUkt2v4U/edit?usp=sharing. Let me know if you have any trouble accessing it. Everyone should be able to add comments and suggestions.

cc @Shopify/cloudplatform

[benlangfeld]

Excellent, thanks @KnVerey . Here's what I propose as an approach for attacking this, each step being a PR with the intent of being merged before moving on to the following step (to reduce rebase overhead) and the possibility of being included in intermediate releases:

1. Add krane CLI in parallel with the existing CLI, as close to a pure addition as possible.
2. Add support for new annotation names, retaining backward compatibility with (but raising warnings on) the old annotation names.
3. Rename internals (class names, filenames).
4. Rename the project (gemspec).
5. Remove backward compatibility (old annotations, old CLI) some number of weeks later.

Does this sound reasonable to you?

[dturn]

Overall this sounds like a good approach. 2 minor concerns:

• Having to do something like require 'kubernetes-deploy'; Krane::DeployTask.new is odd. We may want to merge step 3 and step 4 at the same time.
• (5) It's possible we'll want to support the old annotations longer than the old CLI, so we may want to break those apart, but I think we can discuss that when you're ready to start step 5.

[dturn]

Krane 1.0.0 has been released tracking removal of annotations in #526