Add an option to raise on failure #71
Conversation
achamo
commented
Apr 21, 2023
achamo
commented
- When the max attempt of max_failures is reached, we usually do nothing and move on
- We might want to fail the run instead of waiting forever or ignore the failure
achamo
force-pushed
the
raise_on_failure
branch
from
828d557 to
d55e5fa
Compare
* Please the linter Change-Id: Iff1fca422e80b7d4a9c451ebcd92b0299d8cae7e
achamo
force-pushed
the
raise_on_failure
branch
3 times, most recently
from
923326d to
d19b5b3
Compare
libraries/primitive_consul_lock.rb
Outdated
|
|
||
| def wait_until(action, opts = {}) | ||
| dc = "(in #{@options[:datacenter]})" if @options[:datacenter] | ||
| opts[:max_failures] ||= @options[:max_failures] |
There was a problem hiding this comment.
You are updating the users' input, this is not super great in term of side effect. Could you store it in a local variable?
libraries/primitive_consul_lock.rb
Outdated
| Chef::Log.info "#{action.to_s.capitalize}ed the lock #{path}" | ||
| else | ||
| Chef::Log.warn "Will ignore errors and since we've reached #{opts[:max_failures]} errors" | ||
| raise "Max attempt #{options[:max_failures]} reached" if @options[:max_failures] && !opts[:continue] |
There was a problem hiding this comment.
You are mixing @options & options here (use a local variable :D)
achamo
force-pushed
the
raise_on_failure
branch
from
d19b5b3 to
2fed84f
Compare
| Chef::Log.info "Will #{action} the lock #{path} #{dc}" | ||
| start = Time.now | ||
| success = 0.upto(opts[:max_failures] || Float::INFINITY).any? do |tries| | ||
| max_failures = opts[:max_failures] || @options[:max_failures] |
There was a problem hiding this comment.
OPT: || Float::INFINITY could be added on this line
libraries/primitive_consul_lock.rb
Outdated
| else | ||
| Chef::Log.warn "Will ignore errors and since we've reached #{opts[:max_failures]} errors" | ||
| Chef::Log.warn "Will ignore errors and since we've reached #{max_failures} errors" | ||
| raise "Max attempt #{max_failures} reached" if max_failures && !opts[:continue] |
There was a problem hiding this comment.
OPT: For max_failures you now have a global option (@options) but you didn't do the same for continue is it expected? Maybe add a global option.
Mandatory: the implementation used to not raise, now it will fail except if the option is specified, this is a breaking change, for a not super critical feature.
I think you should either invert the flag fail_on_max_failures (default false) or configure a default value of true to your new continue option.
OPT: document this new feature somehow
NIT: continue is not super explicit continue_on_failure would longer but be more explicit.
Tip: If you add a global option you could define a local variable as you did for max_failures, it would also give you a trivial opportunity to define the default value to true
achamo
force-pushed
the
raise_on_failure
branch
from
2fed84f to
92c6636
Compare
* When the max attempt of max_failures is reached, we usually do nothing and move on * We might want to fail the run instead of waiting forever or ignore the failure * The max_failures is not user defined, we let them define the limit and raising if they do Change-Id: I32719795edb5fbbd081442f54419ff3450e8c5f8
achamo
force-pushed
the
raise_on_failure
branch
from
92c6636 to
d95db01
Compare
| start = Time.now | ||
| success = 0.upto(opts[:max_failures] || Float::INFINITY).any? do |tries| | ||
| max_failures = opts[:max_failures] || @options[:max_failures] | ||
| raise_on_failures = opts[:raise_on_failures].nil? ? @options[:raise_on_failures] : opts[:raise_on_failures] |
There was a problem hiding this comment.
NIT: the .fetch method is great for that e.g. opts.fetch(:raise_on_failures, @options[:raise_on_failures])
