Skip to content
This repository has been archived by the owner on Dec 27, 2022. It is now read-only.

Boolean properties are overriden by default values when set to false #489

Open
bodgix opened this issue Feb 16, 2018 · 15 comments · May be fixed by #490
Open

Boolean properties are overriden by default values when set to false #489

bodgix opened this issue Feb 16, 2018 · 15 comments · May be fixed by #490

Comments

@bodgix
Copy link

bodgix commented Feb 16, 2018

This snippet from providers/instance.rb demonstrate the issue:

@create_account = new_resource.create_account || Logstash.get_attribute_or_default(node, @name, 'create_account')

when new_resource.create_account is set to false, Logstash.get_attribute_or_default(node, @name, 'create_account') is getting invoked because this is how the || operator works.

I think that an explicit check if the property is nil should be used instead of the simplified check if it's truthy.

bodgix added a commit to bodgix/chef-logstash that referenced this issue Feb 16, 2018
This commit fixes a bug where boolean properties
of the `instance’ resource were getting overriden
by default values when they were set to false.

Fixes lusis#489

Signed-off-by: Bogdan Katynski <[email protected]>
@bodgix bodgix linked a pull request Feb 16, 2018 that will close this issue
@martinb3
Copy link
Collaborator

martinb3 commented Feb 16, 2018

When new_resource.create_account is set to false, these should return the same thing, right?

new_resource.create_account || Logstash.get_attribute_or_default(node, @name, 'create_account')
new_resource.create_account.nil? || Logstash.get_attribute_or_default(node, @name, 'create_account')

Is the issue that someone has set create_account on the node object? I'd say that's a logic error in the cookbook doing it.

@bodgix
Copy link
Author

bodgix commented Feb 16, 2018

no the two return different things.

the first one calls

Logstash.get_attribute_or_default(node, @name, 'create_account')

which reads the value of the node['logstash']['instance_default']['create_account'] which is set to true in attributes/default.rb

rendering setting this property to false impossible.

The second line, does not call Logstash.get_attribute_or_default(node, @name, 'create_account') and the value of new_resource.create_account remains false

@martinb3
Copy link
Collaborator

Wouldn't it be easier to simply override the node attribute, if you're going to use one and default to true?

@martinb3
Copy link
Collaborator

(or get rid of the node attribute)

@bodgix
Copy link
Author

bodgix commented Feb 16, 2018

Ok but what's the point of letting the user set a resource property if it's getting overriden anyway and is always true?

To me this is clearly a bug.

@bodgix
Copy link
Author

bodgix commented Feb 16, 2018

I've just spent an hour trying to figure out why

logstash_instance 'name' do
   ...
   create_account false
end

is trying to create the account.

@martinb3
Copy link
Collaborator

Having a resource property and looking at the node object allows us to support both styles of usage (resources vs. recipes). But if someone tries to mix them, there's actually lots of ways in which this will break (separate resource collections & run contexts, chefspec, chef-shell, etc).

I've just spent an hour trying to figure out why

From what I understand, your situation is actually:

default['logstash']['name']['create_account'] = true

logstash_instance 'name' do
   ...
   create_account false
end

Is that more accurate? I feel like that's inconsistent. I'd almost rather raise an error there.

@bodgix
Copy link
Author

bodgix commented Feb 16, 2018

no. I do not set

default['logstash']['name']['create_account'] = true

I'm only calling the resource.

@martinb3
Copy link
Collaborator

martinb3 commented Feb 16, 2018

@bodgix can you show me a minimal example? I suspect something is setting node['logstash']['name']. otherwise both the property and the utility method should return false.

@martinb3
Copy link
Collaborator

Ah, I see what's happening. We should probably adjust the utility method to never return true if node['logstash'][@name] isn't set.

@bodgix
Copy link
Author

bodgix commented Feb 16, 2018

IMHO the utility method should only be called IFF the property isn't set.

@martinb3
Copy link
Collaborator

martinb3 commented Feb 16, 2018

IMHO the utility method should only be called IFF the property isn't set.

My concern is that it's a backwards-incompatible change. We should just drop support for the node object entirely if we do that 👍 (and make this a library cookbook, like it should be.)

@bodgix
Copy link
Author

bodgix commented Feb 16, 2018

well but the utility method isn't called for String properties when they're set.

The problem is the use of || operator. It's works as expected for String/Array because strings or arrays can only be falsey when they're nil.

Boolean are special and this is why I propose to drop the use of

property || default_property

For booleans.

@bodgix
Copy link
Author

bodgix commented Feb 16, 2018

the root of the problem is that an unset boolean and a boolean set to false are both falsey. and I believe that the utility function should only be called for unset properties. and it still will be with my change.

@bodgix
Copy link
Author

bodgix commented Feb 17, 2018

Thank you for pointing me to the workaround.

Since my patch may break backward compatibility, I decided to try it.

I set ['logstash']['instance'][instance_name]['create_account'] to false in my recipe so now I have:

node.override['logstash']['instance'][instance_name]['create_account'] = false

logstash_instance instance_name do
  base_directory base_dir
  install_type 'tarball'
  version '1.5.6'
  source_url "file://#{File.join(base_dir, 'logstash-1.5.6.tar.gz')}"
  checksum '812a809597c7ce00c869e5bb4f87870101fe6a43a2c2a6586f5cc4d1a4986092'
  create_account false
  user 'some_user'
  group 'some_group'
end

but the provider is still trying to create the user.

I believe there is another problem and it's in the utility method this time:

instance_attr || default_attr

I believe that the intention of the utility method is to return the instance_attr if it's set or the default_attr otherwise. The problem is that when the instance_attr is set to false, this expression becomes

false || default_attr

which evaluates to default_attr.

I finally managed to disable account creation by overriding

node.default['logstash']['instance_default']['create_account'] = false

in my recipe.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants