Unify render errors and handle an edge case #253

KnVerey · 2018-03-19T23:19:54Z

Started off fixing an edge case where a .yml file that actually starts with some ERB would raise an exception instead of failing gracefully with a nice message. Ended up shaving a small yak upon noticing that we displayed errors about invalid templates several different ways.

Before

test_deploy_fails_if_required_binding_not_present

test_missing_nested_partial_prints_helpful_error

test_invalid_yaml_in_partial_prints_helpful_error

test_invalid_yaml_fails_fast

test_multiple_invalid_k8s_specs_fails_on_apply_and_prints_template

After

test_deploy_fails_if_required_binding_not_present

test_missing_nested_partial_correctly_identifies_invalid_template_and_its_parents [improved test]

test_invalid_yaml_in_partial_prints_helpful_error

test_invalid_yaml_fails_fast

test_multiple_invalid_k8s_specs_fails_on_apply_and_prints_template

KnVerey · 2018-03-19T23:24:49Z

test/integration/kubernetes_deploy_test.rb

@@ -865,4 +879,8 @@ def test_cronjobs_can_be_deployed
    cronjobs = FixtureSetAssertions::CronJobs.new(@namespace)
    cronjobs.assert_cronjob_present("my-cronjob")
  end
+
+  def test_friendly_error_on_misidentified_erb_file


This is the new one:

KnVerey · 2018-03-19T23:28:12Z

.rubocop.yml

@@ -3,3 +3,9 @@ inherit_from:

 AllCops:
  TargetRubyVersion: 2.3
+
+Style/TrailingCommaInArrayLiteral:


I'm not opposed to adding these (I guess it must have been added to the styleguide? rubocop started caring about it), but doing so now would cause rebase hell for the several people with huge PRs in flight.

Maybe we flip these on when we hit 1.0? I agree these are going to cause a lot of pain for nothing more than a style preference right now.

I personally like enjoy trailing commas in hashes and arrays, they make it easier to add an element, treat every line the same way and they show up as only one line change in git. So I am all for enabling them. But yeah, we shouldn't bug people with open PRs with it.

dturn

Some minor comments. Though I wonder if we want to check for the presence of <% and if its there add something liker "Did you forgot to add .erb suffix?" to the warning message

dturn · 2018-03-22T13:59:04Z

lib/kubernetes-deploy/deploy_task.rb

@@ -225,24 +225,25 @@ def discover_resources
        end
      end
      resources
+    rescue InvalidTemplateError => e
+      debug_msg = paragraph_for_invalid_templates(e.message, filenames: [e.filename], content: e.content)
+      @logger.summary.add_paragraph(debug_msg)


Do you envision a case where you'd want generate the message and not log it? If not can you combine these two lines into a function log_invalid_templates ?

Why log here instead of split_templates

Why log here instead of split_templates

I think I did that because one of the three places that can raise this is the rescue of split_templates, so the alternative is:

def split_templates(filename) ... rescue Psych::SyntaxError => e log_invalid_templates(e.message, filenames: [filename], content: rendered_content) raise FatalDeploymentError, "Failed to render and parse template" rescue InvalidTemplateError => e log_invalid_templates(e.message, filenames: [e.filename], content: e.content) raise FatalDeploymentError, "Failed to render and parse template" end

It's a bit of duplication, but I don't feel strongly about it. Do you think that would be easier to follow?

I think moving it to split_templates is easer to read / understand.

dturn · 2018-03-22T13:59:14Z

.rubocop.yml

@@ -3,3 +3,9 @@ inherit_from:

 AllCops:
  TargetRubyVersion: 2.3
+
+Style/TrailingCommaInArrayLiteral:


Maybe we flip these on when we hit 1.0? I agree these are going to cause a lot of pain for nothing more than a style preference right now.

klautcomputing

I only have minor comments, I need to run the tests and take a look at the output before approving it though :)

klautcomputing · 2018-03-25T00:55:30Z

.rubocop.yml

@@ -3,3 +3,9 @@ inherit_from:

 AllCops:
  TargetRubyVersion: 2.3
+
+Style/TrailingCommaInArrayLiteral:


I personally like enjoy trailing commas in hashes and arrays, they make it easier to add an element, treat every line the same way and they show up as only one line change in git. So I am all for enabling them. But yeah, we shouldn't bug people with open PRs with it.

klautcomputing · 2018-03-25T01:02:27Z

lib/kubernetes-deploy/deploy_task.rb

-      debug_msg += "> Error from kubectl:\n#{indent_four(err)}"
-      if file_content.present?
-        debug_msg += "\n> Rendered template content:\n#{indent_four(file_content)}"
+    def paragraph_for_invalid_templates(err, filenames:, content:)


I think it's a little weird to have one required argument, followed by two required keyword arguments.
Especially since the function has an if content present? block which kind of implies that content is not always required. Maybe make all 3 keyword parameters and give content: a default of nil? or just do the latter?

klautcomputing · 2018-03-25T02:25:01Z

lib/kubernetes-deploy/renderer.rb

-      raise InvalidPartialError, err.message
+      # get the filename from the first parent, not the missing partial itself
+      raise err if err.filename == partial
+      raise InvalidPartialError.new(err.message, filename: partial, content: expanded_template || template)


Shouldn't the underlying code raise the correct error type? Instead of rescuing one type and raising either one or the other?

This is transforming the data, not just the error type. rescue PartialNotFound needs to be distinct because it is the first parent (potentially of a long chain) that has the relevant filename and template content. So we hit this case in the missing child (in which case we skip it, L64) and the first parent (in which case we grab the filename and content, L65), and then after that we just add the additional parent name to the list (L67), like we would for any other type of error.

KnVerey

In making the change to have paragraph_for_invalid_templates do the logging itself, I noticed that record_invalid_template could be simplified and merged with it. It was more complex because it supported multiple file paths, but really, calling it once per file path (by refactoring into what is now record_apply_failure) leads to better output anyway. Please take another look.

The main tests affected by the last commit are test_multiple_invalid_k8s_specs_fails_on_apply_and_prints_template, test_output_when_switching_labels_incorrectly and test_invalid_k8s_spec_that_is_valid_yaml_but_has_no_template_path_in_error_prints_helpful_message

KnVerey · 2018-03-27T23:48:41Z

lib/kubernetes-deploy/deploy_task.rb

+        "You may wish to roll back this deploy."
+      @logger.summary.add_paragraph(ColorizedString.new(warn_msg).yellow)
+
+      unidentified_errors = []


This might be a bad idea. If there were a multi-line error message where one of the middle lines has a filename in it, we'd be splitting it up strangely. I haven't seen that, but I certainly haven't seen everything. An alternative is to always dump the complete error before the individual cases we were able to extract and enhance.

I would totally say, let's deal with it when it comes up

KnVerey · 2018-03-27T23:52:17Z

lib/kubernetes-deploy/deploy_task.rb

+      line.scan(%r{"(/\S+\.ya?ml\S*)"}).each_with_object([]) do |matches, bad_files|
+        matches.each do |path|
+          content = File.read(path) if File.file?(path)
+          bad_files << { filename: File.basename(path), err: line, content: content }


This lets us split up the various errors we've identified appropriately.

Example before this commit:

Example after:

dturn · 2018-03-29T14:40:17Z

LGTM, I'm not worried about the possible edge case with multiline errors and filenames.

klautcomputing · 2018-04-06T17:51:55Z

lib/kubernetes-deploy/deploy_task.rb

+        "You may wish to roll back this deploy."
+      @logger.summary.add_paragraph(ColorizedString.new(warn_msg).yellow)
+
+      unidentified_errors = []


I would totally say, let's deal with it when it comes up

KnVerey force-pushed the render_error branch from bd9cddf to 2c8d545 Compare March 19, 2018 23:21

KnVerey commented Mar 19, 2018

View reviewed changes

KnVerey force-pushed the render_error branch from 2c8d545 to 7cf2c7b Compare March 19, 2018 23:26

KnVerey commented Mar 19, 2018

View reviewed changes

KnVerey requested review from klautcomputing and dturn March 21, 2018 22:58

dturn approved these changes Mar 22, 2018

View reviewed changes

klautcomputing reviewed Mar 25, 2018

View reviewed changes

KnVerey commented Mar 27, 2018

View reviewed changes

klautcomputing approved these changes Apr 6, 2018

View reviewed changes

KnVerey force-pushed the render_error branch from f1083eb to 2348c89 Compare April 6, 2018 23:43

KnVerey added 3 commits April 17, 2018 19:12

Unify render errors and handle an edge case

a4b064e

Fix invalid template identification for partials

e384dc6

Improve/merge record_invalid_template

e9d7ca7

KnVerey force-pushed the render_error branch from 2348c89 to e9d7ca7 Compare April 17, 2018 23:22

KnVerey merged commit 6ee85ce into master Apr 17, 2018

KnVerey deleted the render_error branch April 17, 2018 23:39

stefanmb had a problem deploying to rubygems April 18, 2018 16:51 Failure

stefanmb deployed to rubygems April 18, 2018 16:52 Active

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Unify render errors and handle an edge case #253

Unify render errors and handle an edge case #253

KnVerey commented Mar 19, 2018 •

edited

Loading

KnVerey Mar 19, 2018

KnVerey Mar 19, 2018 •

edited

Loading

dturn Mar 22, 2018

klautcomputing Mar 25, 2018

dturn left a comment

dturn Mar 22, 2018

dturn Mar 22, 2018

KnVerey Mar 27, 2018

dturn Mar 27, 2018

dturn Mar 22, 2018

klautcomputing left a comment

klautcomputing Mar 25, 2018

klautcomputing Mar 25, 2018

klautcomputing Mar 25, 2018

KnVerey Mar 27, 2018

KnVerey left a comment

KnVerey Mar 27, 2018

klautcomputing Apr 6, 2018

KnVerey Mar 27, 2018

dturn commented Mar 29, 2018 •

edited

Loading

klautcomputing Apr 6, 2018

Unify render errors and handle an edge case #253

Unify render errors and handle an edge case #253

Conversation

KnVerey commented Mar 19, 2018 • edited Loading

Before

After

Choose a reason for hiding this comment

KnVerey Mar 19, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

klautcomputing 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

KnVerey 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

dturn commented Mar 29, 2018 • edited Loading

Choose a reason for hiding this comment

KnVerey commented Mar 19, 2018 •

edited

Loading

KnVerey Mar 19, 2018 •

edited

Loading

dturn commented Mar 29, 2018 •

edited

Loading