-
Notifications
You must be signed in to change notification settings - Fork 0
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
Conversation
46f92e7
to
3d2beb5
Compare
3bfe562
to
2ad7acb
Compare
There was a problem hiding this 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' |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
STDERR.puts "Consult: Skipping missing template: #{name}" | |
STDERR.puts "Consult: Skipping missing key: #{name}" |
?
There was a problem hiding this comment.
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?
There was a problem hiding this 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
3d2beb5
to
c9a299b
Compare
- 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
2ad7acb
to
798b070
Compare
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: