Skip to content

Update cfndsl requirement from < 1.0 to ~> 1#356

Merged
orien merged 7 commits intomasterfrom
orien/upgrade-cfndsl
Feb 3, 2021
Merged

Update cfndsl requirement from < 1.0 to ~> 1#356
orien merged 7 commits intomasterfrom
orien/upgrade-cfndsl

Conversation

@orien
Copy link
Copy Markdown
Member

@orien orien commented Jan 24, 2021

Context

I notice we're not allowing cfndsl 1.0 or later. It would be good to rely on the latest stable release.

While exploring an upgrade I found that we're missing Cucumber tests for the compile command.

Change

  • Add Cucumber features for the compile command. Cover Sparkle Formation and CFN DSL templates.
  • Update cfndsl requirement from < 1.0 to ~> 1


def perform
puts(proposed_stack.template_body)
StackMaster.stdout.puts(proposed_stack.template_body)
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Print the compiled template to the StackMaster configured STDOUT.

This allows the printed content to be captured during Cucumber tests, so we can write assertions for it.

end

def self.compile(template_dir, template, compile_time_parameters, _compiler_options = {})
CfnDsl.disable_binding
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This option has been removed in later versions of cfndsl.

Comment on lines -12 to +13
::CfnDsl.eval_file_with_extras(template_file_path).to_json
json_hash = ::CfnDsl.eval_file_with_extras(template_file_path).as_json
JSON.pretty_generate(json_hash)
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Use a pretty format for the compiled template. This is consistent with how we compile Sparkle Formation templates.

end

JSON.pretty_generate(sparkle_template)
JSON.pretty_generate(sparkle_template.dump)
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I noticed the compiled SparkleFormation template wasn't always pretty formatted during Cucumber tests.

We avoid this problem by dumping the template to a hash before converting it to a JSON string.


EC2_Instance(:MyInstance) {
DisableApiTermination external_parameters["DisableApiTermination"]
DisableApiTermination external_parameters.fetch(:DisableApiTermination, "false")
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

In V1, cfndsl doesn't allow compile time parameters to be missing. We need to provide a default.

expect {
compile_time_parameters.delete("DisableApiTermination")
}.to change { JSON.parse(compile)["Resources"]["MyInstance"]["Properties"]["DisableApiTermination"] }.from('true').to(nil)
}.to change { JSON.parse(compile)["Resources"]["MyInstance"]["Properties"]["DisableApiTermination"] }.from('true').to('false')
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Test the default is used, when the compile time parameter is not provided.

@orien orien marked this pull request as ready for review January 24, 2021 22:27
@orien orien changed the title Update cfn-nag requirement from < 1.0 to ~> 1 Update cfndsl requirement from < 1.0 to ~> 1 Jan 24, 2021
Copy link
Copy Markdown
Contributor

@runlevel5 runlevel5 left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Copy Markdown
Contributor

@petervandoros petervandoros left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@orien
Copy link
Copy Markdown
Member Author

orien commented Feb 2, 2021

@redterror are you still using cfndsl? How does this PR look to you?

Copy link
Copy Markdown
Contributor

@patrobinson patrobinson left a comment

Choose a reason for hiding this comment

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

Approved on the proviso we at least make an attempt to let users know this potentially breaking change is coming.

@redterror
Copy link
Copy Markdown
Contributor

@orien I skimmed, it seems reasonable to me. Thanks for flagging, I'll update my repos using cfndsl once this is merged and report back if I hit any snags.

@orien
Copy link
Copy Markdown
Member Author

orien commented Feb 3, 2021

Thanks @redterror. I appreciate it.

@orien
Copy link
Copy Markdown
Member Author

orien commented Feb 3, 2021

@patrobinson I've updated the change log entry to warn that cfndsl version 1 potentially has breaking changes.

@patrobinson
Copy link
Copy Markdown
Contributor

:shipit:

@orien orien merged commit 072b0fa into master Feb 3, 2021
@orien orien deleted the orien/upgrade-cfndsl branch February 3, 2021 06:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants