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

Add a Krane CLI with one command #524

Merged
merged 9 commits into from
Sep 10, 2019
Merged

Add a Krane CLI with one command #524

merged 9 commits into from
Sep 10, 2019

Conversation

dturn
Copy link
Contributor

@dturn dturn commented Aug 13, 2019

What are you trying to accomplish with this PR?

Start down the road of adding a new CLI Krane. Our first step is adding krane version

See #256 for lots of details

How is this accomplished?
We've picked Thor to help make the cli as easy as possible. We've picked version to be the first command because it's just about trivial.

What could go wrong?
Its new code, so the only real issue is if the dependency causes people headaches.

CLI

➜  kubernetes-deploy git:(cli) ✗ be krane version         
[INFO][2019-08-14 11:06:56 -0700]	krane 0.26.7

➜  kubernetes-deploy git:(cli) ✗ be exe/krane  
Krane commands:
  krane help [COMMAND]  # Describe available commands or one specific command
  krane version         # Prints the version

➜  kubernetes-deploy git:(cli) ✗ be exe/krane version
[INFO][2019-08-13 16:43:39 -0700]	krane 0.26.7

➜  kubernetes-deploy git:(cli) ✗ be exe/krane version -v
ERROR: "krane version" was called with arguments ["-v"]
Usage: "krane version"

A fun question here is what exactly should the output be. I looked at kubectl, ruby, node, bash, and grep and each of these prints something different. Most are of the form program_name version_string. So that's what I went for.

@dturn dturn force-pushed the cli branch 2 times, most recently from 6ffc725 to df12046 Compare August 14, 2019 00:06
Rakefile Outdated Show resolved Hide resolved
@dturn dturn requested review from KnVerey and timothysmith0609 and removed request for KnVerey August 14, 2019 00:11
Copy link
Contributor

@timothysmith0609 timothysmith0609 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just need some clarification on the __FILE__ == $PROGRAM_NAME logic in exe/krane since it wasn't quite working as I would have expected.

exe/krane Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
Rakefile Outdated Show resolved Hide resolved
lib/krane/cli/base.rb Outdated Show resolved Hide resolved
@timothysmith0609
Copy link
Contributor

Oops, didn't notice the failed CI before I 👍 but it's just a simple rubocop fix

@timothysmith0609 timothysmith0609 dismissed their stale review August 19, 2019 16:28

Changes since I approved, going to start review over

lib/krane/cli/version_command.rb Outdated Show resolved Hide resolved
test/exe/krane_test.rb Outdated Show resolved Hide resolved
Copy link
Contributor

@KnVerey KnVerey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had a look at this yesterday and chatted with Danny. Here's a quick summary of my thoughts:

  • I'm not a big fan of concerns, especially when they aren't being introduced to allow code reuse. In this case we're basically just using them to hide the code in another file. Let's make proper objects instead if we want to separate pieces of the code. I played around a bit, and this is my suggestion for an alternative: cli...knverey_cli (I'm not suggesting adding the deploy task in the same PR btw--just trying to get a feel for how something larger would look).
  • I noticed while playing around with the branch that Thor automatically introduces two versions for boolean arguments, e.g. if you add --prune you also get --no-prune. That's not according to plan, but I'm fine with it.
  • I also noticed that -f path/to/file -f path/to/other ends up with just path/to/other in the filename option. Maybe I'm doing something wrong, but that could be a problem. Let's confirm whether this works before merging anything with Thor. If it doesn't, you'll need to make a call on whether that's a dealbreaker (or maybe it could be PR'd upstream?).
  • With a single CLI, we're always immediately loading all of our code, and that is visibly slow. Especially when all you're trying to do is print the help for a command. We should make sure the way we structure our code leaves us options for speeding this up. The obvious/easy/sketchy way would be to move the actual requires right into the command-invoking methods (or the thing they call). Maybe there's a better way though.
  • Danny and I talked about having a blackbox test for each command that invokes the actual executable and basically just checks exit codes.
  • Seeing an instance_variable_set anywhere is always a red flag for me (ivars are very internal details of a class!), but it is true that we have no way to inject a logger in this case. In theory the CLI classes themselves should not need to use our fancy logger though. Most of the time they shouldn't be printing anything at all themselves (version may be the one exception). Since these classes are getting their own small test suite, I'd suggest considering using the standard capture_io methods and running that suite in serial (those methods aren't threadsafe IIIRC). Here's an example that wraps capture_io to continue to provide the PRINT_LOGS option and expose the logs to helpers.

Copy link
Contributor

@timothysmith0609 timothysmith0609 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall this makes sense to me. I imagine we'll learn more as we start adding more commands, but this seems like a proper start. Just a few small outstanding questions before I ✅

lib/krane/cli/version_command.rb Outdated Show resolved Hide resolved
test/exe/krane_test.rb Outdated Show resolved Hide resolved
@@ -0,0 +1,5 @@
# frozen_string_literal: true
require 'kubernetes-deploy/common'
Copy link
Contributor Author

@dturn dturn Sep 3, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a partial optimization to make loading time faster. If we require kubernetes-deploy it takes about 0.8 seconds to print the krane help. As is about 0.4 and if we require nothing 0.3. I feel like this strikes a good balance between not feeling to slugish to print help but not having to cherry pick exactly what we want when want it.

@dturn dturn requested review from timothysmith0609 and a team September 3, 2019 16:12
@dturn
Copy link
Contributor Author

dturn commented Sep 3, 2019

This is ready for 👀 though I don't want to merge it till after the -f flag PR. I played with what it might look like to add the --check flag in e6306af and feel good that there is a reasonable way forward for more than just trivial commands. @KnVerey took a stab at deploy in cli...knverey_cli and this pr took some but not all of the ideas.

Copy link
Contributor

@RyanBrushett RyanBrushett left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me

@dturn dturn requested a review from a team September 9, 2019 15:34
Copy link
Contributor

@dirceu dirceu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me!

Copy link
Contributor

@timothysmith0609 timothysmith0609 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:

@dturn dturn merged commit 3fbc68b into master Sep 10, 2019
@dturn dturn deleted the cli branch September 10, 2019 16:58
@ghost ghost added the krane [ProdX-GSD] label Nov 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants