`kubectl -f`-style support for deploy task #514

timothysmith0609 · 2019-07-23T16:46:11Z

What are you trying to accomplish with this PR?

Implement a new flag, -f, with similar semantics to kubectl -f
- -f accepts a comma-separated list of filenames and/or directories
- Must support proper partials scoping for both file and directory arguments
- Must work with templates passed from stdin (tbh this is mostly for free)
Currently this is only for deploy_task, but the -f parsing is done in such a way as to be reusable (via TemplateDiscovery#templates)
Required some changes to method signatures in the tests. I tried to make these as minor as possible.
--template-dir is still supported (both at the CLI and API level), but should be considered deprecated once this PR merges

Update (Aug 15, 2019)
The scope of this PR expanded from just supporting multiple template directory to full fledged support for passing in both files and directories. This approach necessitated a few changes to some internal abstractions, namely TemplateDiscovery and the new LocalResourceDiscovery.

TemplateDiscover::templates(paths) returns a mapping of the form dir -> [filenames]. As an example, let's say the paths argument is [my/dir, other/foo.yml] and the contents of my/dir and other are [bar.yml, baz.yml] and [foo.yml, more.yml]. The return value of templates will thus be:

{
  "my/dir": ["bar.yml", "baz.yml"],
  "other": ["foo.yml"]
}

TemplateDiscovery performs proper aggregation of resources and eliminates the possibility of duplicate values existing. Furthermore, proper partial scoping is preserved since we maintain a mapping of directories for all kinds of resources (both dirs and files).

How is this accomplished?
Initially, I tried passing multiple template directories around deploy_task, but this became cumbersome: I had to create a mapping of template directories to their respective partials dirs, wrap them together, and pass them to and from TemplateContext objects and back to the (somewhat-global) renderer.

A cleaner approach was to simply instantiate a separate renderer for each template directory passed in, then, when appropriate, concatenate the result of each template directory rendering operation and proceed through the deploy as normal. This way, the changes to the codebase are much smaller and the logic for each individual element stays simpler.

Edit: What I ended up doing in the end is a bit of a mix of the first 2 ideas. In the deploy task constructor, we declare a mapping of renderers to template directories. The renderers are then lazily instantiated directly at the point of rendering (in #split_templates), leveraging the closure provided by the default block of the @renderers Hash (thank you Blake for the great suggestion).

Please see the unresolved comments from my review for more fine-grained issues regarding this PR.

What could go wrong?
How defensive should we be? I think that giving people the ability to specify multiple template directories places the responsibility of ensuring they end up with a sane and collision-free experience squarely on them.

TODO

Add tests

timothysmith0609

A few points for discussion

lib/kubernetes-deploy/options_helper.rb

lib/kubernetes-deploy/renderer.rb

lib/kubernetes-deploy/deploy_task.rb

lib/kubernetes-deploy/renderer.rb

lib/kubernetes-deploy/deploy_task.rb

test/integration/kubernetes_deploy_test.rb

timothysmith0609

Some tidying up and implementation of Blake's suggestion + a few more comments for discussion

timothysmith0609 · 2019-07-24T14:23:29Z

lib/kubernetes-deploy/render_task.rb

@@ -29,7 +29,7 @@ def run!(stream, only_filenames = [])
      @logger.phase_heading("Initializing render task")

      filenames = if only_filenames.empty?
-        TemplateDiscovery.new(@template_dir).templates
+        TemplateDiscovery.new([@template_dir]).templates[@template_dir]


This is the result of the change in contract of TemplateDiscovery#new requiring an array of template dirs.

Do we want the render task to accept multiple template dirs. Note that the render task can also accept an arbitrary list of filenames as a filter. For multiple template dirs, we'll just apply the filter to each directory.

If we do want the render task to accept multiple template directories as well, do we want to tackle that in this PR or in a follow-up?

I don't think that accepting multiple directories makes sense the way the renderer's CLI is currently implemented. For example, imagine you have a setup like this:

dirA/ one.yml two.yml dirB/ one.yml two.yml

And what you want to render is dirA/one.yml and dirB/two.yml. You... couldn't?

According to our design for the Krane CLI, we will want both commands to use a -f argument that behaves similarly to kubectl's. That strikes me as incompatible with a directory/filter system: in that design, we'd get a mixed list of files and directories, and have to group individual files given to us by (inferred) deploy directory. Given the importance of the directory to us, maybe the -f plan doesn't make sense after all, because it would lead to unintuitive behaviour. WDYT?

In any case, I'm thinking it would be a breaking API change for the render CLI (unlike the deploy CLI), and if that's the case, we should save it for the transition to Krane.

I think you're absolutely right. The current semantics of kubernetes-render are incompatible with a kubectl -f-like model. For instance, there would be no way to know which bare file names provided to kubernetes-render point to which provided template dirs :/

To me, this means we need to make a fundamental change to the CLI contract of kubernetes-render. In particular, dropping support for providing bare filenames as arguments and instead force the (conventional) semantics of the -f flag. From your above example, this would be:
krane render -f dirA/one.yml,dirB/two.yml (not that bad).

In light of this, I'd like to keep kubernetes-render as-is (only supporting a single template dir), and make a follow-up PR to make krane render behave with -f semantics.

timothysmith0609 · 2019-07-24T14:25:23Z

test/helpers/fixture_deploy_helper.rb

  # Exposed for direct use only when deploy_fixtures cannot be used because the template cannot be loaded pre-deploy,
  # for example because it contains an intentional syntax error
-  def deploy_dir(dir, **args)
+  def deploy_dir(*dirs, **args)


Multiple template directories are exceptional, so it seemed more straightforward to change the signature to accept an arbitrary number of dirs as opposed to enforcing an actual array. I can do the latter if that's what we ultimately want.

timothysmith0609 · 2019-07-24T14:27:09Z

test/integration/kubernetes_deploy_test.rb

+  end
+
+  def test_deploy_successful_with_multiple_template_dirs_multiple_partials
+    result = deploy_dir(fixture_path("test-partials"), fixture_path("test-partials2"),


The goal of this test is to ensure that partials with identical names are actually rendered according to the relative path of the given template directory. This was done by using an identically-named partial that renders different parts of the spec; if the 'wrong' partial is used, the deploy would fail. This convinces me enough that the renderer for a given template directory is behaving as expected (e.g. properly scoped to the right dir)

dturn · 2019-07-24T21:33:51Z

My original concern with this PR was how will it interact with our goal of supporting a -f flag in the Krane CLI https://docs.google.com/document/d/1oInUsKplYGNWTymPY48xtDCx3x4Cwc-phcEKUkt2v4U/edit#. I think this actually makes it easier @timothysmith0609 thoughts?

timothysmith0609 · 2019-07-25T13:57:19Z

My original concern with this PR was how will it interact with our goal of supporting a -f flag in the Krane CLI https://docs.google.com/document/d/1oInUsKplYGNWTymPY48xtDCx3x4Cwc-phcEKUkt2v4U/edit#. I think this actually makes it easier @timothysmith0609 thoughts?

At the very least, I don't think this PR makes things any more difficult in achieving that end. The sole complicating factor between kubectl -f and krane deploy -f is our support of partials. I think there's a few ways we can tackle that:

When given a directory/directories, proceed as we currently do (this PR actually enables this behaviour)
When given concrete filenames (not directories), collect them together by directory, and treat those collections as more template_dirs (this will require more thought than just this, of course).

lib/kubernetes-deploy/deploy_task.rb

lib/kubernetes-deploy/options_helper.rb

KnVerey · 2019-07-26T18:38:26Z

lib/kubernetes-deploy/render_task.rb

@@ -29,7 +29,7 @@ def run!(stream, only_filenames = [])
      @logger.phase_heading("Initializing render task")

      filenames = if only_filenames.empty?
-        TemplateDiscovery.new(@template_dir).templates
+        TemplateDiscovery.new([@template_dir]).templates[@template_dir]


I don't think that accepting multiple directories makes sense the way the renderer's CLI is currently implemented. For example, imagine you have a setup like this:

dirA/ one.yml two.yml dirB/ one.yml two.yml

And what you want to render is dirA/one.yml and dirB/two.yml. You... couldn't?

According to our design for the Krane CLI, we will want both commands to use a -f argument that behaves similarly to kubectl's. That strikes me as incompatible with a directory/filter system: in that design, we'd get a mixed list of files and directories, and have to group individual files given to us by (inferred) deploy directory. Given the importance of the directory to us, maybe the -f plan doesn't make sense after all, because it would lead to unintuitive behaviour. WDYT?

In any case, I'm thinking it would be a breaking API change for the render CLI (unlike the deploy CLI), and if that's the case, we should save it for the transition to Krane.

lib/kubernetes-deploy/deploy_task.rb

KnVerey · 2019-07-26T19:00:35Z

test/fixtures/test-partials2/partials/pod.yaml.erb

+  name: web-from-partial
+spec:
+  replicas: 1
+  template:


Nitpick: only this part is a podspec, despite the name of the file.

The goal of the test is to ensure that it's not loading the identically named partial in the other directory, so I kept the partial name the same but ensured the render would fail to validate if it grabbed the wrong partial

KnVerey · 2019-07-26T19:17:48Z

lib/kubernetes-deploy/deploy_task.rb

@@ -102,22 +102,24 @@ def server_version
      kubectl.server_version
    end

-    def initialize(namespace:, context:, current_sha:, template_dir:, logger:, kubectl_instance: nil, bindings: {},
+    def initialize(namespace:, context:, current_sha:, template_dirs:, logger:, kubectl_instance: nil, bindings: {},


I was trying to think of ways to avoid this breaking API change, but the options that occur to me aren't pretty:

Keep template_dir:, but let it accept an array and do @template_dirs = Array(template_dir) in the initializer

have both template_dir:, and template_dirs:, either the initializer merges them or it raises an error if they're both specified

It kinda irks me to make a breaking API change like this soon before we make another one (with Krane, which is a single clean cutover in a way). I'd be kinda ok with something like the second option above as a result. If we keep it breaking though, let's make it very prominent in the changelog.

test/unit/kubernetes-deploy/options_helper_test.rb

dturn · 2019-07-26T23:28:59Z

A cleaner approach was to simply instantiate a separate renderer for each template directory passed in, then, when appropriate, concatenate the result of each template directory rendering operation and proceed through the deploy as normal. This way, the changes to the codebase are much smaller and the logic for each individual element stays simpler.

Edit: What I ended up doing in the end is a bit of a mix of the first 2 ideas.

Can you comment on why the cleaner approach didn't work out?

timothysmith0609 · 2019-07-29T19:59:14Z

Can you comment on why the cleaner approach didn't work out?

The renderer takes a few arguments that are easily grabbed in the task initializer. In the end, it seemed easier to wrap those in the closure provided by the default hash block than try to pass them into TemplateDiscovery further down the stack. That's really the only difference

dturn · 2019-08-14T16:37:02Z

Perhaps TemplateDiscovery is poorly named, and should instead be something like ResourceDiscovery. ResourceDiscovery is then responsible for finding all relevant resource files in a template directories, splitting the templates in each file, calling the renderer on them, and returning a list of fully realized KubernetesResources.

I can't comment in the thread but, I like the idea. As an fyi we have a class called ClusterResourceDiscovery so I'd prefer something more specific LocalResouceDiscovery?

dturn

I like this a lot better, thanks for putting in the work to refactor it!

lib/kubernetes-deploy/deploy_task.rb

test/fixtures/ejson-cloud2/secrets.ejson

timothysmith0609

This is ready for a (hopefully final) review

test/unit/kubernetes-deploy/options_helper_test.rb

timothysmith0609 · 2019-08-15T15:23:36Z

lib/kubernetes-deploy/deploy_task.rb

      @namespace = namespace
      @namespace_tags = []
      @context = context
      @current_sha = current_sha
-      @template_dir = File.expand_path(template_dir)
+      @template_dirs = (template_dirs.map { |dir| File.expand_path(dir) } << template_dir).compact


We don't need to worry about the case of template_dir and template_dirs both existing when invoking via CLI, but I've done it this way to make it accommodate the case of someone using the API itself.

timothysmith0609 · 2019-08-15T15:25:50Z

lib/kubernetes-deploy/deploy_task.rb

+          errors << "Template directory `#{template_dir}` doesn't exist"
+        elsif Dir.entries(template_dir).none? { |file| file =~ /(\.ya?ml(\.erb)?)$|(secrets\.ejson)$/ }
+          errors << "`#{template_dir}` doesn't contain valid templates (secrets.ejson or postfix .yml, .yml.erb)"
+        end


This whole block is another thing that should be extracted out of DeployTask if possible

lib/kubernetes-deploy/local_resource_discovery.rb

timothysmith0609 · 2019-08-15T17:44:40Z

lib/kubernetes-deploy/template_discovery.rb

+          hash[dir_name] << File.basename(filename) unless hash[dir_name].include?(filename)
+        end
+
+        template_paths


Now any task should be able to call TemplateDiscovery#templates and receive back a mapping of dirname: [files]. The input of templates is an array whose elements are either file paths or directories (abs and/or relative). The nice thing here is that there's zero other dependencies for generating the dir->files mappings

timothysmith0609

Implemented the TemplateSets abstraction I talked about with @dturn . It actually ends up cleaning things up quite a bit since we now have a single API interface to deal with arbitrary numbers of template paths.

I've added unit tests and fleshed out the validation for TemplateSets.

Also realized a silly error I made and corrected it: -f flag can now be invoked multiple times

README.md

lib/kubernetes-deploy/ejson_secret_provisioner.rb

lib/kubernetes-deploy/template_sets.rb

dturn

I really like this refactor, my only real question is the params to new_from_dirs_and_files.

lib/kubernetes-deploy/deploy_task.rb

lib/kubernetes-deploy/ejson_secret_provisioner.rb

lib/kubernetes-deploy/options_helper.rb

lib/kubernetes-deploy/template_sets.rb

dturn

Let's be sure to 🎩 this before merging

lib/kubernetes-deploy/template_sets.rb

Different approach with multiple renderers rubocop change ejson_secret_provisioner to handle multiple template dirs rejig tests due to method signature/name changes more test updates actually a tab: use \t instead of actual tab (for linter) fixup refactor to avoid passing template_dirs all around hacky fix for render task, add test for partial naming collision test result, not forced failure deploy_dir -> deploy_dirs Add assert_logs_match tests to multi-template-dir tests ensure ejson secrets from multiple template directories are created rubocop fixes avoid nilref error possibility in OptionsHelper#default_template_dir force unique template dirs support template_dir (deprecated) and template_dirs TemplateDiscovery -> ResourceDiscovery rubocop pass namespace properly pass filename from raise InvalidTemplateException reraise Psych::SyntaxError as InvalidTemplateError error handling dance yet more error handling ResourceDiscovery -> LocalResourceDiscovery whitespace tidy up DeployTask#discover_resources reject(&nil?) -> compact optionshelper test + refactor rubocop tidy up exe/kubernetes-deploy for easier deprecation + checking the return of template discovery + proper -f handling (files + dirs) tests for filename handling as arg lots of renaming deal with ejsonsecretprovisioner case. more naming fixes TemplateDiscovery cleanup testing partials, secrets, with filenames lint test make render cli work. rename some flag fields CHANGELOG handling ejsonsecretprovisioning with new template args statsh Move back to multiple ejsonprovisioners. EjsonProvisioner must be supplied a secret, it no longer fetches one itself fix File.join bug VALID_EXTENSIONS shouldn't include secrets.ejson remove unused memo for TemplateDicovery::resource_templates (method is now a class method, not instance) use fixture_path instead of relative path for test. Remove redundant condition from default_template_dir in options_helper rubocop README some pr review fix render task no-template-dir-provided case. Pass ejson file, not simply dirname stash changes stash working template_set implementation (need to rejig tests) rubocop + basename(filename) fix for record_invalid_template" dealing with secrets, however sloppily validations beginnings stash fix rendering error not propogating message properly more dir validation fix test remove period code cleanup remove unused code ensure we use config/deploy/$ENVIRONMENT if no template_dir(s) given

…teSet objects

…ling name as a file, not a dir. Need to fail fast on OptionsHelper instead

Katrina is off on vacation and this has gone through peer review in her absence

timothysmith0609 commented Jul 23, 2019

View reviewed changes

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

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

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

timothysmith0609 changed the title ~~Initial implementation of multiple template dirs support~~ -WIP- Initial implementation of multiple template dirs support Jul 23, 2019

timothysmith0609 force-pushed the support_multiple_template_dirs branch 2 times, most recently from 57cabbe to 79d77dc Compare July 23, 2019 19:49

timothysmith0609 commented Jul 23, 2019

View reviewed changes

lib/kubernetes-deploy/deploy_task.rb Show resolved Hide resolved

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

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

timothysmith0609 requested review from BlakeMesdag and KnVerey July 23, 2019 19:58

BlakeMesdag reviewed Jul 23, 2019

View reviewed changes

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

timothysmith0609 commented Jul 24, 2019

View reviewed changes

test/integration/kubernetes_deploy_test.rb Outdated Show resolved Hide resolved

timothysmith0609 commented Jul 24, 2019

View reviewed changes

timothysmith0609 force-pushed the support_multiple_template_dirs branch from 19f2e3d to a60dbcf Compare July 24, 2019 14:28

timothysmith0609 requested a review from dturn July 24, 2019 14:28

timothysmith0609 changed the title ~~-WIP- Initial implementation of multiple template dirs support~~ Initial implementation of multiple template dirs support Jul 24, 2019

timothysmith0609 changed the title ~~Initial implementation of multiple template dirs support~~ Multiple template dirs support for deploy task Jul 24, 2019

timothysmith0609 self-assigned this Jul 24, 2019

timothysmith0609 added the 🆕 feature Makes something new possible label Jul 24, 2019

timothysmith0609 force-pushed the support_multiple_template_dirs branch 2 times, most recently from 001722e to 78b72bd Compare July 26, 2019 12:39

KnVerey reviewed Jul 26, 2019

View reviewed changes

dturn reviewed Aug 14, 2019

View reviewed changes

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

test/fixtures/ejson-cloud2/secrets.ejson Outdated Show resolved Hide resolved

dturn mentioned this pull request Aug 14, 2019

krane render #529

Closed

timothysmith0609 commented Aug 15, 2019

View reviewed changes

test/unit/kubernetes-deploy/options_helper_test.rb Show resolved Hide resolved

timothysmith0609 commented Aug 15, 2019

View reviewed changes

timothysmith0609 changed the title ~~Multiple template dirs support for deploy task~~ kubectl -f-style support for deploy task Aug 15, 2019

timothysmith0609 commented Aug 30, 2019

View reviewed changes

README.md Outdated Show resolved Hide resolved

lib/kubernetes-deploy/ejson_secret_provisioner.rb Show resolved Hide resolved

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

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

dturn reviewed Aug 30, 2019

View reviewed changes

dturn approved these changes Aug 30, 2019

View reviewed changes

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

RyanBrushett approved these changes Sep 3, 2019

View reviewed changes

timothysmith0609 added 21 commits September 4, 2019 12:14

using template sets

e5ffa10

bit of fix

dc26244

pass render_erb flag through

8d193f8

ejsonsecretprovisioner mapping

e8c4201

typo

018f434

templatesets

61a610d

test?

615ea27

templatesets unit tests

a7c5937

one more unit test

c520686

don't use each_with_object

594816c

lint

8b3cd2d

update CONTRIBUTING.md

8ced984

support additive -f flags

5a55541

PR comments

4a2732f

#templates uses presence of @Renderer to decide whether or not to render

1f6dd7b

get rid of TemplateSets#<<, use private ctor accepting array of Tempa…

237681c

…teSet objects

README: note that -f can be invoked multiple times

8573605

if default_template_dir does not exist, we erroneously treat the trai…

ab0f7a6

…ling name as a file, not a dir. Need to fail fast on OptionsHelper instead

Don't use @kubectl in DeployTask, use the getter method instead

c463236

rebase on master'

790f010

timothysmith0609 force-pushed the support_multiple_template_dirs branch from 6c9cbd4 to 790f010 Compare September 4, 2019 16:20

timothysmith0609 merged commit c2e92f1 into master Sep 4, 2019

timothysmith0609 deleted the support_multiple_template_dirs branch September 4, 2019 17:19

timothysmith0609 mentioned this pull request Sep 4, 2019

v0.27.0 #545

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`kubectl -f`-style support for deploy task #514

`kubectl -f`-style support for deploy task #514

timothysmith0609 commented Jul 23, 2019 •

edited

Loading

timothysmith0609 left a comment

timothysmith0609 left a comment

timothysmith0609 Jul 24, 2019

KnVerey Jul 26, 2019

timothysmith0609 Aug 15, 2019

timothysmith0609 Jul 24, 2019

timothysmith0609 Jul 24, 2019

dturn commented Jul 24, 2019

timothysmith0609 commented Jul 25, 2019

KnVerey Jul 26, 2019

KnVerey Jul 26, 2019

timothysmith0609 Jul 26, 2019

KnVerey Jul 26, 2019

dturn commented Jul 26, 2019

timothysmith0609 commented Jul 29, 2019

dturn commented Aug 14, 2019

dturn left a comment

timothysmith0609 left a comment

timothysmith0609 Aug 15, 2019

timothysmith0609 Aug 15, 2019

timothysmith0609 Aug 15, 2019

timothysmith0609 left a comment •

edited

Loading

dturn left a comment

dturn left a comment

kubectl -f-style support for deploy task #514

kubectl -f-style support for deploy task #514

Conversation

timothysmith0609 commented Jul 23, 2019 • edited Loading

timothysmith0609 left a comment

Choose a reason for hiding this comment

timothysmith0609 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

dturn commented Jul 24, 2019

timothysmith0609 commented Jul 25, 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

dturn commented Jul 26, 2019

timothysmith0609 commented Jul 29, 2019

dturn commented Aug 14, 2019

dturn left a comment

Choose a reason for hiding this comment

timothysmith0609 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

timothysmith0609 left a comment • edited Loading

Choose a reason for hiding this comment

dturn left a comment

Choose a reason for hiding this comment

dturn left a comment

Choose a reason for hiding this comment

`kubectl -f`-style support for deploy task #514

`kubectl -f`-style support for deploy task #514

timothysmith0609 commented Jul 23, 2019 •

edited

Loading

timothysmith0609 left a comment •

edited

Loading