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 configuration to allow skipping missing template files/keys #48

Merged
merged 2 commits into from
Oct 3, 2023

Conversation

aharpervc
Copy link
Contributor

This PR introduces skip_missing_template: true optional config for templates, which allows configured templates to not exist. This builds in the feature that you'd overwise have to work around with ERB logic in the consult config file itself. Eg, it replaces this:

    <% path = File.join("./config/templates/#{ENV['AWS_REGION']}.json.erb") %>
    <% if File.exist? path %>
    region-config:
      path: config/templates/<%= ENV['AWS_REGION'] %>.json.erb
      dest: config/region.json
    <% else %>
    <% puts "Consult: No #{ENV['AWS_REGION']}.json.erb present, skipping..." %>
    <% end %>

Copy link

@henrythach henrythach left a comment

Choose a reason for hiding this comment

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

LGTM

end

def consul_contents(location)
[@config[location]].compact.flatten.map do |key|
Diplomat::Kv.get(key, options: {}, not_found: :return, found: :return).force_encoding 'utf-8'

Choose a reason for hiding this comment

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

I assume you had to remove the not_found and found args here in order to implement the rescue logic?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed in this commit message: 28220a8

Choose a reason for hiding this comment

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

thanks for the clarification

Diplomat::Kv.get(key, {}, :reject, :return).force_encoding 'utf-8'
rescue Diplomat::KeyNotFound
if @config[:skip_missing_template]
STDERR.puts "Consult: Skipping missing template: #{name}"

Choose a reason for hiding this comment

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

Suggested change
STDERR.puts "Consult: Skipping missing template: #{name}"
STDERR.puts "Consult: Skipping missing key: #{name}"

?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm, currently the thing being skipped is the template. Maybe it'd be better to include the file path or consul key causing the skip, so you know what's missing?

Copy link

@ronnietaylor ronnietaylor left a comment

Choose a reason for hiding this comment

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

My questions and comments are non-blocking. Before those this LGTM

Base automatically changed from mkdir-for-rendered-files to master October 3, 2023 22:06
- this method doesn't have keyword arguments, so the parameters were being interpreted as the options hash
- since `contents` is called multiple times and does work, it's now memoized
@aharpervc aharpervc merged commit 844e766 into master Oct 3, 2023
3 checks passed
@aharpervc aharpervc deleted the skip-missing-template branch October 3, 2023 22:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants