Forbidden optional parameters set to nil when not present in command line

[eguzki]

Currently, forbidden optional parameters set to nil when not present in command line. From documentation, it is expected to return false

option :f, :force, 'push with force', argument: :forbidden

run do |opts, args, cmd|
  puts "Force? #{opts[:force].inspect}!"
end

% ./push
Force? nil!

% ./push --force
Force? true!

[denisdefreyne]

Good catch — that is indeed supposed to be false (although in practice, nil and false behave quite similarly.)

Will fix!

[eguzki]

Sure!. But my use case made the difference:

def attrs
  {
    force: opts[:force]
  }.compact
end

[denisdefreyne]

This is now released in 2.15.7.

[eguzki]

Thanks for the quick reaction 🥇

[scotje]

It looks like previously the keys weren't set at all in the opts hash and now they are set to false.

Not requesting any change to this fix, just a warning to people who may have previously been checking for the existence of the key (e.g. opts.has_key?(:force)) as the behavior of a check like that will now have changed. (That was never a very safe way to check by itself anyway. :))

[denisdefreyne]

@scotje Oh no, that’s unfortunately. I do believe that the new (current) behavior is more correct, but “correctness” is a gray area now.

I feel that now is a good time for me to set up an integration test with r10k, because there have been several issues with Cri <-> r10k lately, which I naturally want to avoid at all costs.

[scotje]

This particular issue was with PDK (which is the primary Cri-dependent tool I work with these days).

You can see the fix here, it was honestly mostly with our tests being overly strict about options hash being passed around: puppetlabs/pdk#672

[pcarlisle]

In r10k we need to distinguish between the cli being invoked without a flag and being invoked with an explicit false. Is there an alternative way to do this?

[denisdefreyne]

@pcarlisle

In r10k we need to distinguish between the cli being invoked without a flag and being invoked with an explicit false. Is there an alternative way to do this?

At the moment, there no longer is.

Can you elaborate on where you need this behavior?

I have a few ideas on how this behavior could be made available again without breaking anything else, though I’ll need a bit of time to implement it.

[denisdefreyne]

I will reopen this ticket because there are still issues with it.

[denisdefreyne]

Here’s an enhancement which I believe makes this situation a bit more clear: #99. It should meet @eguzki’s requirements as well as be useful for Puppet.

@pcarlisle Can you check whether this makes sense to you?

option :a, :animal, 'add animal', default: 'giraffe', argument: :required

run do |opts, args, cmd|
  puts "Animal = #{opts[:animal]}"
  puts "Option given? #{opts.key?(:animal)}"
end

% ./run --animal=donkey
Animal = donkey
Option given? true

% ./run --animal=giraffe
Animal = giraffe
Option given? true

% ./run
Animal = giraffe
Option given? false

[denisdefreyne]

Note that #99 does require a small change to pdk’s tests, which I’ve prepared: puppetlabs/pdk#674

[pcarlisle]

Can you elaborate on where you need this behavior?

Yes, we have a configuration file which can be overridden by command line options and we only wish to do that when they are explicitly passed.

The change in #99 looks like it would work for this.

[denisdefreyne]

This is now fixed in the 2.15.8 release.

Note that the behavior has changed slightly: it is now more closely aligned with the behavior in 2.15.6, and better defined. See the 2.15.8 release notes for details.

[alexjfisher]

I think there was still a breaking change in here somewhere. See voxpupuli/onceover#256