Support stdin #415

timothysmith0609 · 2019-01-22T14:50:33Z

What are you trying to accomplish with this PR?
Closed #410
Required for #229

Allow kubernetes-deploy and kubernetes-render to accept input from STDIN instead of only accepting a directory path.

How is this accomplished?
~~Add a new flag to deploy and render: --use-stdin (cannot be combined with --template-dir)~~
~~If --use-stdin is set, create a temporary directory and write a yml.erb file to it (just one file for the entire input for simplicity's sake)~~

The tricky part of this PR was figuring out how to get automatic cleanup (e.g. Using the block form of Dir.mktmpdir) without having to deal too much with the other case (using --template-dir). I did this by wrapping the tasks inside lambdas. If --use-stdin is set, an additional wrapper lambda is used to take care of setup and teardown of the temporary files/directories. I think this is preferable to trying to maintain a reference to a tempdir that we have to then manually cleanup afterwards (particularly in the face of errors and unexpected exits).

Edit: Instead of adding a new flag, --template-dir can now be set multiple times in an invocation. When set to -, it will read the contents of STDIN. In the case of multiple template dirs or reading from STDIN, the templates will all be written to a temporary directory to consolidate all the templates in a single location (keeping the logic in each task runner simple).

Incidental bugfix

I noticed that the docs for kubernetes-render were not accurate with regards to defaults for template-dir. As it stands, kubernetes-render will not use the default config/deploy/$ENVIRONMENT path if --template-dir is not set. This PR corrects that deficiency.

What could go wrong?
Unexpected IO errors, possible issues with keeping all templates in a single file (maybe??)

dturn

I don't like the use of Procs. Seems like we could do the clean-up in an ensure block at the end of the rescues with less complication than this. Part of me also doesn't like the need for an explicit flag. But given the implicit behavior of using config/deploy/$ENVIRONMENT not sure we want to muddy the waters.

timothysmith0609 · 2019-01-22T17:38:34Z

given the implicit behavior of using config/deploy/$ENVIRONMENT not sure we want to muddy the waters.

We can probably revisit this when we're ready to switch the major version and can introduce such a breaking change.

There's two commits for this PR now: With Lambdas and Without Lambdas. My preference is still to use the initial version (lambdas) since proper cleanup is guaranteed. Without them, I feel that the error handling is a little clumsier. For instance, KubernetesDeploy::OptionsHelper.default_and_check_template_dir could raise and that has the potential to leak directories. Conversely, not ensuring template_dir in the ensure clause might yield confusing ENOENT errors. Maybe I'm worrying too much about rare edge cases, though

KnVerey · 2019-02-06T00:24:34Z

exe/kubernetes-deploy

 begin
+  template_dir = KubernetesDeploy::OptionsHelper.default_and_check_template_dir(template_dir, use_stdin: use_stdin)


Of the two options, I think this one is a lot easier to follow overall. However, I agree that it is a better design to make the place that creates the temp dir responsible for cleaning it up--callers should not have to know what the options helper did! Maybe we could get both advantages by modifying this version to yield the validated path instead of returning it:

begin KubernetesDeploy::OptionsHelper.with_validated_template_path(template_dir, use_stdin: use_stdin) do |valid_path| runner = KubernetesDeploy::DeployTask.new(template_dir: valid_path, ...) runner.run! end rescue KubernetesDeploy::DeploymentTimeoutError exit(70) rescue KubernetesDeploy::FatalDeploymentError exit(1) end

def self.with_validated_template_dir(template_dir, use_stdin: false) if use_stdin # ...validate and make temp dir yield temp_dir return end if !template_dir && ENV.key?("ENVIRONMENT") yield "config/deploy/#{ENV['ENVIRONMENT']}" elsif !template_dir || template_dir.empty? raise FatalDeploymentError, "Template directory is unknown. " \ "Either specify --template-dir argument or set $ENVIRONMENT to use config/deploy/$ENVIRONMENT " \ + "as a default path." else yield template_dir end nil ensure FileUtils.remove_entry(temp_dir) if temp_dir end

This turned out simpler than I thought it would be and should avoid any manual cleanup outside of a bit of error handling since we can encompass everything in the mktmpdir block, it will clean everything up for us on exit.

KnVerey · 2019-02-06T00:29:34Z

exe/kubernetes-deploy

@@ -23,6 +26,7 @@ ARGV.options do |opts|
  opts.on("--allow-protected-ns", "Enable deploys to #{prot_ns}; requires --no-prune") { allow_protected_ns = true }
  opts.on("--no-prune", "Disable deletion of resources that do not appear in the template dir") { prune = false }
  opts.on("--template-dir=DIR", "Set the template dir (default: config/deploy/$ENVIRONMENT)") { |v| template_dir = v }
+  opts.on("--use-stdin", "Use $stdin instead of a template dir") { use_stdin = true }


I agree with Danny that it isn't ideal to have an additional flag if we can help it. Could we use the unix convention of - representing stdin? Ultimately, it could be nice to make this arg feel more like kubectl's, which uses --filename for both files and directories, can combine multiple instances of the option into a list, and accepts - to mean stdin.

That sounds reasonable. So you'd like to be able to use --template-dir to be used multiple times (or comma-separated values, I suppose). WDYT about the following 2 options:

1- Serially call kubernetes-deploy for each template-dir
2- When multiple template-dirs exist and/or stdin is used, place the contents of all the template dirs + stdin in a temp directory and call deploy_task just once.

I'm leaning towards 2, myself.

Option 2 👍
Pruning would cause problems for option 1.

...files and directories, can combine multiple instances of the option into a list, and accepts - to mean stdin.

It turns out ARGF supports this out of the box 👍

exe/kubernetes-deploy

lib/kubernetes-deploy/options_helper.rb

KnVerey · 2019-02-06T00:33:24Z

lib/kubernetes-deploy/options_helper.rb

-    def self.default_and_check_template_dir(template_dir)
+    def self.default_and_check_template_dir(template_dir, use_stdin: false)
+      if use_stdin
+        if $stdin.tty?


Can you explain this check to me? Intuitively I'd expect $stdin to be a tty under normal usage, when this command is being invoked from the command line.

$stdin will be a tty when not reading from a pipe. So in our case, if we specify to use stdin but there's no pipe to read from, we'll return an error

Changed this to check for length of ARGV instead. This gives us more flexibility for extra filename arguments as well as STDIN support via -

lib/kubernetes-deploy/options_helper.rb

timothysmith0609 · 2019-02-21T02:15:41Z

^^ Example of deploying the hello-cloud fixture reading from stdin

Added unit tests and took @KnVerey 's suggestion to yield the template directory to the runner.

cc @Shopify/cloudx this is ready for 👀

benlangfeld · 2019-02-27T11:25:32Z

Would someone be so kind as to paste the test failures that were encountered in buildkite here so that I may attempt to resolve them?

ghost · 2019-02-27T14:05:57Z

Test failures

Finished tests in 1.276781s, 158.9936 tests/s, 773.0377 assertions/s.
  |  
  |  
  | Failure:
  | OptionsHelperTest#test_missing_template_dir_no_extra_input [/usr/src/app/test/unit/kubernetes-deploy/options_helper_test.rb:26]
  | Minitest::Assertion: KubernetesDeploy::OptionsHelper::OptionsError expected but nothing was raised.
  |  
  | Failure:
  | OptionsHelperTest#test_multiple_template_dirs [/usr/src/app/test/unit/kubernetes-deploy/options_helper_test.rb:18]
  | Minitest::Assertion: Expected: 18
  | Actual: 19
  |  
  | Failure:
  | OptionsHelperTest#test_single_template_dir_only [/usr/src/app/test/unit/kubernetes-deploy/options_helper_test.rb:8]
  | Minitest::Assertion: --- expected
  | actual | 0s
  | @@ -1 +1 @@
  | -"/usr/src/app/test/fixtures/hello-cloud"
  | +"/tmp/kubernetes-deploy20190222-761-1teyzrb"

benlangfeld · 2019-02-27T15:57:32Z

Thank you @randy-abson. Those tests pass for me locally; is there something else significantly different about the CI build environment, other than presumably being some Linux variant?

KnVerey

A couple little things in line, but mainly:

As discussed on chat, I think we should stick to flag-based directory input (not positional)
I like that we can now handle multiple directories 👍
kubernetes-render needs to handle stdin too
other exes probably need to handle your new OptionsError too

lib/kubernetes-deploy/options_helper.rb

timothysmith0609 · 2019-02-28T16:35:39Z

As discussed on chat, I think we should stick to flag-based directory input (not positional)

Changed to only accept the --template-dir arg, setting - to specify STDIN. STDIN is no longer inferred nor are positional args supported to specify filenames.

I like that we can now handle multiple directories

👍

kubernetes-render needs to handle stdin too

Done

other exes probably need to handle your new OptionsError too

Deploy and render are the only 2 exes that need to worry about this; they both now rescue the exception

KnVerey · 2019-02-28T21:31:21Z

README.md

 - `--bindings=BINDINGS`: Makes additional variables available to your ERB templates. For example, `kubernetes-deploy my-app cluster1 --bindings=color=blue,size=large` will expose `color` and `size`.
 - `--no-prune`: Skips pruning of resources that are no longer in your Kubernetes template set. Not recommended, as it allows your namespace to accumulate cruft that is not reflected in your deploy directory.
 - `--max-watch-seconds=seconds`: Raise a timeout error if it takes longer than _seconds_ for any
 resource to deploy.
+- `kubernetes-deploy` also supports reading from STDIN. You can do this by using `--template-dir=-`. Example: `cat templates_from_stdin/*.yml | kubernetes-deploy ns ctx --template-dir=- --template-dir=another_template_dir `. Ensure the templates you are reading in are separated by `---` when using this feature.


Since this isn't a separate flag, I'd suggest including it in the --template-dir explanation

Ensure the templates you are reading in are separated by --- when using this feature.

Is this different than if you have multiple templates in the same directory? Why?

Is this different than if you have multiple templates in the same directory? Why?

It happened to me while testing that, while reading in multiple files, a closing --- was missing between the yamls of some files. On reflection the comment is more specific to YAML and not kubernetes-deploy so I'm in favor of dropping the reference.

lib/kubernetes-deploy/options_helper.rb

KnVerey · 2019-02-28T22:01:17Z

lib/kubernetes-deploy/options_helper.rb

+      def populate_temp_dir(temp_dir:, template_dirs:)
+        template_dirs.each do |template_dir|
+          if template_dir == '-'
+            File.open("#{temp_dir}/#{STDIN_TEMP_FILE}", 'w+') { |f| f.print($stdin.read) }


Everywhere you're hardcoding a / in this file we should change to File.join(path, pieces) for compatibility reasons

Should we still check $stdin.tty? like you were before?

We could. I removed it here since that's the convention I see adopted by most command line tools. On the other hand, it's not great UX if the task hangs waiting for input if the user makes an error in the pipeline.

I'm leaning towards raising an exception if STDIN is specified but attached to a tty

lib/kubernetes-deploy/options_helper.rb

test/unit/kubernetes-deploy/options_helper_test.rb

KnVerey · 2019-02-28T22:21:08Z

test/unit/kubernetes-deploy/options_helper_test.rb

+      template_dir_entries = Dir.entries(template_dir)
+      assert_equal(fixture_path_entries.length, template_dir_entries.length)
+      fixture_path_entries.each do |fixture|
+        assert(template_dir_entries.select { |s| s.include?(fixture) })


I think this will always pass, because select will return [] if it never matches, which is truthy. I suspect that's what's happening, because the two files will have different full paths, one being in a tempdir and one being in the original location.

refute(template_dir_entries.select { |template| template.include?(fixture) }.empty?) passes the test, so the logic is working even though the test was initially flawed.

If you feel it's worth massaging this test some more I'll do so but I'm satisfied with it for now.

KnVerey · 2019-02-28T22:22:32Z

test/unit/kubernetes-deploy/options_helper_test.rb

+    old_stdin = $stdin
+
+    input = Tempfile.open("kubernetes_deploy_test")
+    File.open("#{fixture_path('for_unit_tests')}/service_test.yml", 'r') do |f|


Same comment about using File.join.

OTOH would it be simpler to use StringIO instead of actually manipulating files?

Seeing as the tests are already written I'm inclined to leave them be.

Closes #136

Without lambdas Different approach with yielded deploy dir prevent hanging waiting for stream. Tidyup renames More flexible args handling remove debug gem OptionsHelper as singleton class cop most important method at top of file Tests, OptionsError, refactors Automatic corrections for lib/kubernetes-deploy/options_helper.rb Automatic corrections for test/unit/kubernetes-deploy/options_helper_test.rb fix test police Don't use temp file if only using a single template dir and no additional input tidier more refactor More meaningful testing police remove debug file test tweak README Cleaner template dir handling PR comments doc cleanup rename tests README pr comments

timothysmith0609 · 2019-03-04T01:19:33Z

PR comments have been addressed, rebased on master, and removed old references to revision that were caught up in this branch.

dturn

Some minor stuff, looks solid.

exe/kubernetes-deploy

dturn · 2019-03-04T17:49:22Z

test/unit/kubernetes-deploy/options_helper_test.rb

+    end
+  end
+
+  def test_with_default_env_var


do these tests get run in parallel?

From my observations, no.

See this PR description

no, unit tests don't (because a huge portion of them use mocha)

We should use with_env(key, value) from EnvTestHelper instead though

Addressed in #444

dturn · 2019-03-04T17:50:47Z

test/unit/kubernetes-deploy/options_helper_test.rb

+      input.print(f.read)
+    end
+    input.rewind
+    $stdin = input


Can you write to $stdin instead of abusing a global?

From what I gather, no. The best I can do is ensure that old_stdin is reset at the end of the test. I'm not sure how I could even write to $stdin without changing the descriptor itself, regardless.

timothysmith0609

Removed the support for multiple directories due to trickiness dealing with partial support. I'm going to start work on a follow-up PR to reintroduce multiple template dirs, keeping in mind the need for partials support (since they can live in different directories relative to a given template dir).

The upshot is that this code is much simpler. The basic structure is the same, but there's less cases to handle.

CHANGELOG.md

Co-Authored-By: timothysmith0609 <[email protected]>

timothysmith0609 requested review from KnVerey and dturn January 22, 2019 16:15

dturn suggested changes Jan 22, 2019

View reviewed changes

timothysmith0609 changed the title ~~-WIP- Support stdin~~ Support stdin Jan 22, 2019

timothysmith0609 force-pushed the support_stdin branch 3 times, most recently from 1c9c05c to b063bb1 Compare January 22, 2019 17:34

KnVerey suggested changes Feb 6, 2019

View reviewed changes

timothysmith0609 force-pushed the support_stdin branch 2 times, most recently from c1700b3 to 1ae441e Compare February 20, 2019 20:46

KnVerey reviewed Feb 27, 2019

View reviewed changes

lib/kubernetes-deploy/options_helper.rb Outdated Show resolved Hide resolved

lib/kubernetes-deploy/options_helper.rb Outdated Show resolved Hide resolved

timothysmith0609 force-pushed the support_stdin branch from fcc73b9 to 0c8c3dc Compare February 28, 2019 16:22

KnVerey reviewed Feb 28, 2019

View reviewed changes

KnVerey mentioned this pull request Mar 1, 2019

Deploy secrets in DeployTask like other resources #424

Merged

benlangfeld and others added 6 commits March 3, 2019 18:20

Better Shipit TASK_ID integration (deployment_id)

5c4a7b9

Closes #136

Changelog references

c77a195

Fix rebase: remove revision_from_environment

83a4b3e

use File.join and File::SEPARATOR

dbb401b

Add test for no template dir given but $ENVIRONMENT set

5cc5501

timothysmith0609 force-pushed the support_stdin branch from 513c7dd to 5cc5501 Compare March 4, 2019 00:26

timothysmith0609 added 2 commits March 3, 2019 19:47

rebase fix

7a34852

Cleanup revision references

ea645f4

timothysmith0609 requested review from KnVerey and dturn March 4, 2019 01:19

dturn approved these changes Mar 4, 2019

View reviewed changes

timothysmith0609 added 5 commits March 4, 2019 13:20

use logger on OptionsError

029fcba

remove multiple dir support for now

695948d

avoid shadowing template_dir

61a9e6e

remove debug file

141f1bb

rename

b801f5a

timothysmith0609 commented Mar 5, 2019

View reviewed changes

timothysmith0609 added 3 commits March 5, 2019 11:13

render typo

be569db

remove debug file (again)

bba5db7

update changelog

ed895da

KnVerey approved these changes Mar 5, 2019

View reviewed changes

dturn approved these changes Mar 5, 2019

View reviewed changes

Merge branch 'master' into support_stdin

4a6443d

benlangfeld suggested changes Mar 5, 2019

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

benlangfeld reviewed Mar 5, 2019

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

Update CHANGELOG.md

75fb39f

Co-Authored-By: timothysmith0609 <[email protected]>

timothysmith0609 merged commit 97e81c3 into master Mar 5, 2019

timothysmith0609 deleted the support_stdin branch March 5, 2019 18:42

		begin
		template_dir = KubernetesDeploy::OptionsHelper.default_and_check_template_dir(template_dir, use_stdin: use_stdin)

Support stdin #415

Support stdin #415

Conversation

timothysmith0609 commented Jan 22, 2019 • edited Loading

Incidental bugfix

dturn left a comment

Choose a reason for hiding this comment

timothysmith0609 commented Jan 22, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timothysmith0609 commented Feb 21, 2019

benlangfeld commented Feb 27, 2019

ghost commented Feb 27, 2019

benlangfeld commented Feb 27, 2019

KnVerey left a comment

Choose a reason for hiding this comment

timothysmith0609 commented Feb 28, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timothysmith0609 commented Mar 4, 2019

dturn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timothysmith0609 left a comment

Choose a reason for hiding this comment

timothysmith0609 commented Jan 22, 2019 •

edited

Loading

timothysmith0609 commented Feb 28, 2019 •

edited

Loading