fix: Ignore ansible_host: ""

[Glutexo]

Enhancement: Ignoring _rhc.ansiblehost if set to an empty string.

Reason:

The empty string value causes:

• the Ansible host name to be reset in the Inventory by insights-client --ansible_host=;
• a line ansible_host= to be put in the config file.

The former is a job of {state: absent}. An empty string should not cause such a desctructive operation. The latter is ignored by the Client and is equivatent to that line missing.

Result: Consistently with a similar condition of the display_name parameter, an empty string ansible_host is treated as undefined. It's the same behavior as with a null value.

Issue Tracker Tickets (Jira or BZ if any):

• https://issues.redhat.com/browse/CCT-398

[richm]

If we are going to fix the README.md and other behavior for this parameter, we should fix all of the parameters in this role to make them all consistent. For example, starting with https://github.com/linux-system-roles/rhc/blob/main/README.md#role-variables rhc_organization - what is the default value? What does it mean if the user sets the value of this to null or empty (e.g. rhc_organization:) or an empty string, or even some other empty value rhc_organization: []?

IMO, the most important thing is that the user interface/user experience is consistent. It is bad UX if the user needs to know that setting rhc_organization: "" has a different semantic than ansible_host: "" has a different semantic than prefix: "" etc. There may be exceptions to this, but there should be very, very few exceptions, and very, very well documented.

[Glutexo]

Thanks for your response, @richm! I definitely agree that the user experience should be consistent, including the behavior of empty string values.

In this case of Ansible host, if the empty string value is treated as a string, it results in an inconsistent and possibly destructive behavior. It made sense to me to treat it the same way as a null value, because it’s safer and unsurprising. Without the fix, the empty string value would be closer to {state: absent}, which is very explicit. That’s why I decided to add the condition.

I already did that for the display name parameter (https://github.com/linux-system-roles/rhc/pull/166), which is very similar in its nature. An empty string is treated just as null: it is ignored. The README explicitly mentions “null or an empty string”, rather than “empty value”, because that doesn’t hint that other empty values are valid and ignored too.

Behavior of other empty-like values like empty list or empty dictionary is undefined and from my experiments it looks like the most common thing to happen is that they are serialized by Python to non-empty strings like [] or {}, respectively. Although it’s not very useful value, it’s at least not dangerous and does not cause a different action than expected to be triggered. I think we can leave those undocumented if we don’t say “null or empty value” but rather “null or empty string”.

I know close to nothing about the other parameters like rhc_organisation, so I don’t know, where an empty string is actually a valid value, either as a string or something special that is not described by an explicit value like {state: absent}. I guess that in most cases we can consistently treat empty strings the same way as null. (Is prefix the case – does prefix: "" (empty) mean something different than prefix: null (unset)?) For those remaining fields, where "" makes actual sense, this needs to be (as you said) very, very well documented nevertheless. And as mentioned above, explicitly mentioning “empty string” looks better to me than vague “empty value”.

So what do we do? Keep the condition and the documentation and start an effort of making it consistent for all the parameters? Add the condition silently, not updating README? Or don’t add the condition at all, and remove it from the display name parameter as well? I vote for keeping the condition in both and start actively thinking about other parameters. For the README, my preference is to keep the documentation up to date, even though it would reveal inconsistency, which is already present in the behavior.

[richm]

Thanks for your response, @richm! I definitely agree that the user experience should be consistent, including the behavior of empty string values.

In this case of Ansible host, if the empty string value is treated as a string, it results in an inconsistent and possibly destructive behavior. It made sense to me to treat it the same way as a null value, because it’s safer and unsurprising. Without the fix, the empty string value would be closer to {state: absent}, which is very explicit. That’s why I decided to add the condition.

I agree - for all parameter values - an empty string, or other empty value, or null, should be handled with the semantic "if the value of the parameter is empty or null, leave the existing value alone", unless it is an actual valid value of a parameter, in which case it should be documented explicitly in the README and in defaults/main.yml.

I already did that for the display name parameter (#166), which is very similar in its nature. An empty string is treated just as null: it is ignored. The README explicitly mentions “null or an empty string”, rather than “empty value”, because that doesn’t hint that other empty values are valid and ignored too.

For users, it's better to keep it as "empty string" because we don't want to give users the idea that they can put non-string values in there (except for null, or {state: absent}). In the actual implementation, if we use parameter | d("") | length > 0 as the test of "empty", then it will just work for any data type that has a length.

Behavior of other empty-like values like empty list or empty dictionary is undefined and from my experiments it looks like the most common thing to happen is that they are serialized by Python to non-empty strings like [] or {}, respectively.

All of the values "", [], and {}, when passed to value | d("") | length > 0, will return false. The only wrinkle here is that you cannot use null in this context - it will give an error because length is not a valid operation on the null value.

Although it’s not very useful value, it’s at least not dangerous and does not cause a different action than expected to be triggered. I think we can leave those undocumented if we don’t say “null or empty value” but rather “null or empty string”.

Right - the README should refer to string valued parameters as strings, and use terms like "empty string" when describing these parameters.

I know close to nothing about the other parameters like rhc_organisation, so I don’t know, where an empty string is actually a valid value, either as a string or something special that is not described by an explicit value like {state: absent}.

We should find out and determine if all of the parameters abide by the semantic "if the value of the parameter is empty or null, leave the existing value alone" - if so, code and document them all consistently, and explicitly handle exceptional cases.

I guess that in most cases we can consistently treat empty strings the same way as null. (Is prefix the case – does prefix: "" (empty) mean something different than prefix: null (unset)?)

I don't know, but we should investigate and determine how it should be handled, and ensure the code and documentation reflect that.

For those remaining fields, where "" makes actual sense, this needs to be (as you said) very, very well documented nevertheless. And as mentioned above, explicitly mentioning “empty string” looks better to me than vague “empty value”.

Right. The README should use terms like "empty string" not "empty value" when describing string valued parameters, to reinforce with the user that these values should be strings.

So what do we do? Keep the condition and the documentation and start an effort of making it consistent for all the parameters?

Yes.

Add the condition silently, not updating README? Or don’t add the condition at all, and remove it from the display name parameter as well? I vote for keeping the condition in both and start actively thinking about other parameters. For the README, my preference is to keep the documentation up to date, even though it would reveal inconsistency, which is already present in the behavior.

[Glutexo]

Cool! It looks like we are in agreement. ❤️ I can create a ticket (or maybe even an Epic) for the effort of making the conditions and the documentation more robust and consistent.

Can we get this PR reviewed, then? @richm @ptoscano

• It changes the documentation from “empty value” to “empty string”, which we agreed is what we want.
• It adds an empty string condition that is fixes a bug and is consistent at least with the _displayname parameter.

We (or I) can then experiment with value | d("") | length > 0 you suggested. That won’t require updating the documentation.

[richm]

[citest]

[Glutexo]

Thank you for your approval, @richm! Do we need a second one to merge?

[richm]

Thank you for your approval, @richm! Do we need a second one to merge?

@ptoscano ok?