feat: add ansible host parameter to insights configuration #155
Conversation
DuckBoss
force-pushed
the
jajerome/ansible-host
branch
from
d66f3ad
to
29b2e6b
Compare
|
I'll squash the commits after all the conversations are resolved -- thanks for the feedback so far everyone! 👍 |
I can call the role like this: - name: Run
include_role:
name: linux-system-roles.rhc
vars:
rhc_insights:
autoupdate: truein this case, |
Interesting, i would not have thought about it 🤔 TIL, thanks! |
Good catch! Thanks, I've pushed a commit to address this. [18c7c1c] |
|
@DuckBoss update also the README with this new parameter, so we can keep a kind of documentation for it 😄 |
|
One issue I've noticed is that when the ansible host name is unset, it does not reset the ansible host in HBI to the system host name during the collection. // Run playbook using role and set ansible_host to a custom value.
// This sets the new host in the cfg file and triggers a collection to update HBI.
rhc_insights:
ansible_host: test
// Run the playbook again, but unset the ansible_host value.
// ansible host name is removed from the cfg, but HBI remains as 'test'.
rhc_insights:
ansible_host: {state: absent}Are there any ways to resolve this without an update to insights-client? |
Yes, you are missing to register that variable when the host name is removed from the config file. |
DuckBoss
force-pushed
the
jajerome/ansible-host
branch
from
6219ef7
to
0b9ab6d
Compare
|
I've made some larger changes in an effort to address the issues with updating HBI only when changes are detected and resetting HBI when the ansible host is absent in the config. One of the issues with resetting the ansible host in HBI is that a collection did not reset the HBI ansible host if the config had no value, and the only way to reset HBI is by passing an empty value to I've listed some scenarios below: Expected output when the ansible_host has not changed in the cfg:In this situation, if the Expected output when the ansible_host has changed in the cfg to a different value not already in the cfg:Expected output when the ansible_host is absent ( {state: absent} ):Looking for advice or suggestions about this approach and if there's a cleaner way to write the logic for checking the ansible host in the insights-client config. |
|
@DuckBoss I think you described all the possible used scenarios. However I have a concern where the change in HBI is made. All your tests are in a instance that is already registered, but what will happens if you try to set a display-name and update in inventory when that host doesn't exists there? |
|
Thanks for the work so far, and the patience. I gave the PR a number of tests, and it seems that it works well in all the scenarios I tried. I left a couple of minor nits, but other than that this PR seems ready to me. Hence: please integrate the two small changes, rebase it, squash it, and ensure that the commit message and the PR title are nice and explain things well. Thanks! |
DuckBoss
force-pushed
the
jajerome/ansible-host
branch
from
9221323
to
89ea7c1
Compare
Adds the ansible host name option for insights-client into the rhc role. This adds support for setting an ansible host name in HBI during insights-client registration, updating the current ansible host name in HBI if the system is already registered, and resetting the ansible host name if an absent value is provided. Signed-off-by: Jason Jerome <jajerome@redhat.com>
DuckBoss
force-pushed
the
jajerome/ansible-host
branch
from
89ea7c1
to
1161ced
Compare
|
[citest] |
|
[citest bad] |
There was a problem hiding this comment.
The only test failure is in tests_environments, and it due to a regression in upstream Candlepin that broke the subscription-manager detection of environments. This appeared in the latest version, 4.4.3. I will create a PR to pin the candlepin container to the previous version for now.
Since that issue is unrelated to the changes of this PR, then I'm approving it & merging it.
Thanks for the work, @DuckBoss, good job!
| @@ -11,3 +11,9 @@ | |||
| or "Registered" in __rhc_insights_status.stdout | |||
| register: __rhc_insights_unregister_result | |||
| changed_when: true | |||
|
|
|||
| - name: Reset ansible host in config | |||
| lineinfile: | |||
There was a problem hiding this comment.
Can’t the INI file module be used, so it’s not necessary to match the lines manually with a regexp? Would this be sufficent with control characters that need to be escaped?
There was a problem hiding this comment.
Does Ansible have a plugin to handle INI files, so it’s not necessary to match the lines manually with a regexp?
Yes and no - there is the community.general.ini_file module - but we really don't want the role to rely on 3rd party code unless absolutely necessary.
Would this be sufficent with control characters that need to be escaped?
What is the issue?
There was a problem hiding this comment.
Oh. I didn’t know that it’s 3rd party and we don’t want to rely on that.
For the escaping issue. I am not sure if there is any: I don’t know by heard the details of how Python parses INI files. I was just afraid whether there aren’t some control characters that would cause that Python would read the value differently than hot it’s written without escaping.
There was a problem hiding this comment.
I suppose it is possible that someone could provide an ansible_host value with control characters, but that seems highly unlikely to me.
There was a problem hiding this comment.
I agree it’s unlikely. Maybe it’s not worth the effort. I would however expect from an official RHC role to be safe.
| - rhc_insights.ansible_host is defined | ||
| - not rhc_insights.ansible_host is none | ||
| - rhc_insights.ansible_host != __rhc_state_absent | ||
| - rhc_insights.ansible_host != omit |
There was a problem hiding this comment.
I’m not sure whether I understand the logic with the “omit” name. I couldn’t reproduce any scenario, where this would actually do anything. It rather looks to me like something non-standard that can cause bugs and possibly wouldn’t allow me to give a host “omit” as an Ansible name.
There was a problem hiding this comment.
There is a difference between
- rhc_insights.ansible_host != omit
and
- rhc_insights.ansible_host != "omit"
the "magic" Ansible variable omit has a random, unique value
There was a problem hiding this comment.
Oh, I see! I didn’t know that. Thank you! 🙇🏻♂️
I wondered how can I set rhc_insights.ansible_host to this value. Found out I can do {{ omit }}. 😮
There was a problem hiding this comment.
I am not exactly sure how is this different from null, but I guess that it’s an Ansible way of doing things and then we should support omit too.
| - rhc_insights.ansible_host is defined | ||
| - not rhc_insights.ansible_host is none | ||
| - rhc_insights.ansible_host != __rhc_state_absent | ||
| - rhc_insights.ansible_host != omit |
There was a problem hiding this comment.
I am not exactly sure how is this different from null, but I guess that it’s an Ansible way of doing things and then we should support omit too.
| state: present | ||
| line: ansible_host={{ rhc_insights.ansible_host }} | ||
| check_mode: true | ||
| register: __insights_ansible_host_exists |
There was a problem hiding this comment.
The name __insights_ansible_host_exists sounds misleading to me. It is more “matches the rhc_insights.ansible_host value” rather than “exists”. But it may also mean “this exact ansible_host value exists”, in wich case it is ok.
| regexp: "^ansible_host=" | ||
| state: absent | ||
| - name: Remove ansible host in inventory | ||
| shell: insights-client --ansible-host= & wait |
There was a problem hiding this comment.
Running insights-client --ansible-host= really resets the Ansible Host value in the Inventory. Removing it from the config file, or setting it there to ansible_host=, however, doesn’t do so: after an upload, the value remains unchanged.
That means, the two changes do different things:
- The change to the configuration file causes Ansible Host to remain unchanged in the Inventory, regardless of what the stored value is.
- The CLI call causes the Ansible Host value to reset in the inventory.
Is it ok that it is not possible to reset the Ansible Host value with the config file?
There was a problem hiding this comment.
Is it ok that it is not possible to reset the Ansible Host value with the config file?
I'm not sure what you mean - do you mean "without using the rhc role"?
There was a problem hiding this comment.
Yes yes, using plain Insights Client CLI. I am aware that this is not the place to ask, it just came up on my mind.
| ansible_host: "example-host" | ||
| ``` | ||
|
|
||
| Configures the ansible host name with a custom value for the system record |
There was a problem hiding this comment.
| Configures the ansible host name with a custom value for the system record | |
| Configures the Ansible host name with a custom value for the system record |
| ``` | ||
|
|
||
| Configures the ansible host name with a custom value for the system record | ||
| in Host Based Inventory (HBI). This host name is used in playbooks by remediations. |
There was a problem hiding this comment.
| in Host Based Inventory (HBI). This host name is used in playbooks by remediations. | |
| in Host Based Inventory (HBI). This host name is used in playbooks by Remediations. |
|
|
||
| Possible values of this variable: | ||
|
|
||
| * `null` or an empty value: the ansible host name is not changed. |
There was a problem hiding this comment.
What is an empty value? An empty string? The omit magic value?
| * `null` or an empty value: the ansible host name is not changed. | |
| * `null` or an empty value: the Ansible host name is not changed. |
There was a problem hiding this comment.
like this:
rhc_insights:
ansible_host:This is an "empty" value in YAML which is translated to a null - see https://yaml.org/spec/1.2.2/#24-tags "Example 2.21 Miscellaneous"
I say "empty" because, although it looks empty, that there is nothing there, in YAML this means to assign the value null
There was a problem hiding this comment.
I see. I knew about this feature of YAML, it just didn’t come to my mind.
Since the so-called empty value translates to null on the YAML level, it may not be worth mentioning here: the value is still null and the variations of the syntax are a feature of the YAML format, not Ansible or the role.
There was a problem hiding this comment.
I found out that an empty string value yields inconsistent results:
ansible_host=is put into the config file, causing the value not get updated on upload.- The Client is called with
--ansible_host=, removing the value from the Inventory.
Even though this “empty value” refers to the YAML syntax value:, it sounds ambiguous to me. I’d rather not describe features of YAML and be more explicit with empty strings.
| * `null` or an empty value: the ansible host name is not changed. | |
| * `null`: the ansible host name is not changed. |
if we wanted to use an empty string to reset the value, which I don’t prefer, or
| * `null` or an empty value: the ansible host name is not changed. | |
| * `null` or an empty string: the ansible host name is not changed. |
if we chose to consider an empty string an empty value, equivalent to null or omit.
It may be worth mentioning omit too.
| * `null` or an empty value: the ansible host name is not changed. | |
| * `null`, `omit`, or an empty string: the ansible host name is not changed. |
| Possible values of this variable: | ||
|
|
||
| * `null` or an empty value: the ansible host name is not changed. | ||
| * `{state: absent}`: the ansible host name is unset in the insights-client config file and Host Based Inventory (HBI) is updated to use the system host name. |
There was a problem hiding this comment.
| * `{state: absent}`: the ansible host name is unset in the insights-client config file and Host Based Inventory (HBI) is updated to use the system host name. | |
| * `{state: absent}`: the ansible host name is unset in the Insights Client config file and Host Based Inventory (HBI) is updated to use the system host name. |
or
| * `{state: absent}`: the ansible host name is unset in the insights-client config file and Host Based Inventory (HBI) is updated to use the system host name. | |
| * `{state: absent}`: the ansible host name is unset in the insights-client.conf file and Host Based Inventory (HBI) is updated to use the system host name. |
|
|
||
| * `null` or an empty value: the ansible host name is not changed. | ||
| * `{state: absent}`: the ansible host name is unset in the insights-client config file and Host Based Inventory (HBI) is updated to use the system host name. | ||
| * any other string value: the ansible host name is changed in Host Based Inventory (HBI). |
There was a problem hiding this comment.
| * any other string value: the ansible host name is changed in Host Based Inventory (HBI). | |
| * any other string value: the Ansible host name is changed in Host Based Inventory (HBI). |
|
@Glutexo thank you for your interest in the |
| - rhc_insights.ansible_host != omit | ||
| - rhc_insights.ansible_host != __rhc_state_absent |
There was a problem hiding this comment.
Is there any specific reason, why the two conditions are in different order than at the previous task?
| - rhc_insights.ansible_host != omit | |
| - rhc_insights.ansible_host != __rhc_state_absent | |
| - rhc_insights.ansible_host != __rhc_state_absent | |
| - rhc_insights.ansible_host != omit |
| @@ -38,6 +38,62 @@ | |||
| insertafter: "#auto_update" | |||
| line: auto_update={{ rhc_insights.autoupdate | d(true) | bool }} | |||
|
|
|||
| - name: Check ansible host in insights-client config | |||
There was a problem hiding this comment.
| - name: Check ansible host in insights-client config | |
| - name: Check ansible host in Insights Client config |
or
| - name: Check ansible host in insights-client config | |
| - name: Check ansible host in insights-client.conf |
| - rhc_insights.ansible_host != __rhc_state_absent | ||
| - __insights_ansible_host_exists.changed | ||
| block: | ||
| - name: Update ansible host in insights-client config |
There was a problem hiding this comment.
| - name: Update ansible host in insights-client config | |
| - name: Update ansible host in Insights Client config |
or
| - name: Update ansible host in insights-client config | |
| - name: Update ansible host in insights-client.conf |
| state: absent | ||
| - name: Remove ansible host in inventory | ||
| shell: insights-client --ansible-host= & wait | ||
| register: __insights_ansible_host_removed |
There was a problem hiding this comment.
This variable is not used anywhere. Is it necessary to register it? The previous task, too, doesn’t register a return variable.
| register: __insights_ansible_host_removed |
There was a problem hiding this comment.
This variable is used in the test (tests_insights_ansible_host.yml); it is possible to use it because the role was imported the first time making its variables public:
- name: Add ansible_host and register insights
include_role:
name: linux-system-roles.rhc
public: trueThere was a problem hiding this comment.
I see! The “Update ansible host in inventory” task doesn’t need a return value?
There was a problem hiding this comment.
I couldn’t find the usage of the variable in any test. See the search results.
|
@Glutexo thank you for your interest in the |
| @@ -0,0 +1,116 @@ | |||
| # SPDX-License-Identifier: MIT | |||
| --- | |||
| - name: Insights ansible host test | |||
There was a problem hiding this comment.
| - name: Insights ansible host test | |
| - name: Insights Ansible host test |
|
|
||
| - name: Test ansible_host | ||
| block: | ||
| - name: Add ansible_host and register insights |
There was a problem hiding this comment.
| - name: Add ansible_host and register insights | |
| - name: Add ansible_host and register Insights |
|
|
||
| - name: Check ansible_host is set to 'host' in config file | ||
| command: | ||
| grep -ixq "^ansible_host=host" {{ __rhc_insights_conf }} |
There was a problem hiding this comment.
Can’t lineinfile be used here, rather than a grep command?
The RHC role inserts the line in all lowercase. The -i flag is then redundant and without it, the test would be more precise.
The -x flag already triggers a full-string match, equivalent to ^ at the beginning and $ at the end of the pattern. The ^ in the pattern is then redundant.
I’d rather consider single quoted string. Although there is no interpolation or control characters, it would convey a message of an exact string.
Although the Insights Client config path is constant and doesn’t contain any special characters or spaces, paths can contain those. Paths may contain single quotes too, which can’t be properly escaped at all. It may make sense to rather store it in an enviornment variable.
| grep -ixq "^ansible_host=host" {{ __rhc_insights_conf }} | |
| grep -q 'ansible_host=host' {{ __rhc_insights_conf }} |
There was a problem hiding this comment.
Can’t lineinfile be used here, rather than a grep command?
Yes, if there is a substantial benefit in terms of code readability and long term maintainability. Since this is test code (as opposed to code in the tasks/ runtime directory), I'm willing to allow a bit more leniency towards the preference of the original developer.
There was a problem hiding this comment.
I’d rather consider single quoted string. Although there is no interpolation or control characters, it would convey a message of an exact string.
The general Ansible style is to only quote things that require quoting, so I would prefer no quotes at all.
There was a problem hiding this comment.
Although the Insights Client config path is constant and doesn’t contain any special characters or spaces, paths can contain those. Paths may contain single quotes too, which can’t be properly escaped at all. It may make sense to rather store it in an enviornment variable.
Not sure what you mean by environment variable here - where would the value come from? How would it be set for the task? That seems unnecessarily complicated. Note that Ansible has a built-in filter for quoting variables expanded in a shell command - quote - so this should correctly be {{ __rhc_insights_conf | quote }} - maybe with the environment variable comment you were getting at the fact that using any sort of single or double quoting is not guaranteed to work e.g. '{{ __rhc_insights_conf }}' will fail if __rhc_insights_conf (perversely) contains single quote characters. See https://docs.ansible.com/ansible/2.9/user_guide/playbooks_filters.html#id8
There was a problem hiding this comment.
If there is | quote, which I didn’t know about, there is no need for something as complicated as an environment variable. 👍🏻 Cool!
| remediation: absent | ||
| - name: Check ansible_host is set to 'new-host' in config file | ||
| command: | ||
| grep -ixq "^ansible_host=new-host" {{ __rhc_insights_conf }} |
There was a problem hiding this comment.
Dtto here.
| grep -ixq "^ansible_host=new-host" {{ __rhc_insights_conf }} | |
| grep -q 'ansible_host=new-host' {{ __rhc_insights_conf }} |
| remediation: absent | ||
| - name: Check ansible_host has not changed in config file | ||
| command: | ||
| grep -ixq "^ansible_host=new-host" {{ __rhc_insights_conf }} |
There was a problem hiding this comment.
Dtto here.
| grep -ixq "^ansible_host=new-host" {{ __rhc_insights_conf }} | |
| grep -q 'ansible_host=new-host' {{ __rhc_insights_conf }} |
| ansible_host: {state: absent} | ||
| remediation: absent | ||
| - name: Check ansible_host is not present in config file | ||
| lineinfile: |
There was a problem hiding this comment.
See, here lineinfile is used, rather than grep.
| state: absent | ||
| - name: Remove ansible host in inventory | ||
| shell: insights-client --ansible-host= & wait | ||
| register: __insights_ansible_host_removed |
There was a problem hiding this comment.
I couldn’t find the usage of the variable in any test. See the search results.
| command: | ||
| grep -ixq "^ansible_host=new-host" {{ __rhc_insights_conf }} | ||
| changed_when: false | ||
| - name: Change ansible host to a null value (noop) |
There was a problem hiding this comment.
I believe the substasks starting from here test a different scenario. The parent task is named “Test ansible_host changed value after registration”, which is what the first two subtasks do.
These, however, test that the value in the config file remains unchanged when the Ansible variable is set to null. That is similar in nature to the next scenario, where the Ansible variable is set to absent. I suggest extracting those to a separate task.
| - name: Test ansible_host changed value after registration | ||
| block: | ||
| - name: Change ansible host to 'new-host' | ||
| include_role: |
There was a problem hiding this comment.
I wonder. Since the master task “Test ansible host” includes the role and sets rhc_insights.state to present, it registers the system at its very start. This block, including the role again, triggers the registration on an already registered host?
If so, I’m afraid that the test scenario can be possibly affected by the tainted state. In such case it would be necessary to have the test in its own playbook, which I think is generally a good idea when testing an Ansible role.
In this case, the test should manually edit the config file by an lineinfile task, then trigger the registration, and after that check that the value in the config file changed.
There was a problem hiding this comment.
I wonder. Since the master task “Test ansible host” includes the role and sets rhc_insights.state to present, it registers the system at its very start. This block, including the role again, triggers the registration on an already registered host?
No, because the role is smart enough to see that the system is already registered. One important concept in Ansible is idempotency - running the role again with the same arguments should not change the state of the system, and Ansible should report no "changed": true.
If so, I’m afraid that the test scenario can be possibly affected by the tainted state.
I don't think so.
In such case it would be necessary to have the test in its own playbook, which I think is generally a good idea when testing an Ansible role.
But not in this case.
In this case, the test should manually edit the config file by an lineinfile task, then trigger the registration, and after that check that the value in the config file changed.
I don't think that's necessary in this case.
There was a problem hiding this comment.
I see. The first task does the registration, and then, this one tests that ansible_host is updated on an already registered host. Ok!
| register: __test_ansible_host_removed | ||
| failed_when: __test_ansible_host_removed.found | ||
|
|
||
| always: |
There was a problem hiding this comment.
This is executed only once, not after each task in the block, right? What is the point of it being here, then? Just keeping the system clean for other playbooks? I see there are no checks. What if the unregister role misbehaves? Shouldn’t every playbooks run on a pristine system?
There was a problem hiding this comment.
This is executed only once, not after each task in the block, right?
Right.
What is the point of it being here, then? Just keeping the system clean for other playbooks?
That's one purpose. Another purpose is to check the unregistration code (which is already checked elsewhere in this particular test).
I see there are no checks. What if the unregister role misbehaves?
In this particular test, the unregister code is already checked for misbehavior starting at line 102
Plus, we will get errors with subsequent tests if the always: block code did not clean up properly.
Shouldn’t every playbooks run on a pristine system?
Yes and no. One of the problems we have is performance. If every test in every role must run in a new VM, then that's (number of tests) X (VM provisioning time) X (number of platforms) X (number of Ansible versions). In addition there is the expense of VM provisioning. So we make a compromise - roles that have tests that can properly clean up after the test run are run sequentially in the same VM (well, it is a bit more complicated than that) - and roles that cannot cleanup are able to use a single VM for each test. This has worked very, very well for the past 5 years of system roles development.
There was a problem hiding this comment.
I see. Thank you for such detailed explanation. It helped a lot! 🙇🏻♂️
|
@richm Will do! I‘ll use my comments on this closed PR as a list of thinks I wanted to address. Thanks! 🙇🏻♂️ |
Not sure what you mean here. Which test playbook should be split into multiple ones, and how would you do the split?
We have that option in the test frameworks for tests that do not, or are unable to, clean up properly. |
|
There are no tests that would verify that the Ansible Host has been updated in the Inventory: by querying the Inventory API with cURL and checking the values. |
Since the tests here test the role, they limit themselves to ensure the right commands are run, that the config files are tweaked as needed, and so on. Testing that Inventory is updated as requested would effectively mean testing |
|
Good point, Pino! 💪🏻 I don’t however see that the tests would verify that the commands are run. Only changes to the config file are tested. |
| when: | ||
| - rhc_insights.ansible_host is defined | ||
| - not rhc_insights.ansible_host is none | ||
| - rhc_insights.ansible_host != omit |
There was a problem hiding this comment.
This task is triggered also when rhc_insights.ansible_host is set to an empty string. The result is similar to {state: absent}, but not the same.
display_name=is put into the config file. This value is then ignored by the Client, causing no change on upload. The remove task, on the other hand, removes the line entirely.- The client is run with
--ansible-host=, resetting the value in the inventory.
This is an inconsistent state and I believe we should handle the empty string value consciously:
- Either consider it an empty value, equivalent to
omitornull, not causing any changes; - or consider it equivalent to
{state: absent}and triger the proper remove task by it.
I vote for ⑴, because
- I believe that removing a value should be a concious action, which is already conveyed by the special value
{state: absent}; - ⑵ exposes a property of Client’s CLI, where
--ansible_host=is a special way to trigger a reset; the RHC role shields the user from the CLI.
| - rhc_insights.ansible_host != omit | |
| - rhc_insights.ansible_host != "" | |
| - rhc_insights.ansible_host != omit |
| path: "{{ __rhc_insights_conf }}" | ||
| regexp: "^ansible_host=" | ||
| insertafter: "#ansible_host=" | ||
| line: ansible_host={{ rhc_insights.ansible_host }} |
There was a problem hiding this comment.
This results in
ansible_host=to be inserted in the config file, when rhc.ansible_host is set to an empty string. This value is then ignored by the client, not making any change to the value in the Inventory on upload.
I suggest treating an empty string in rhc.ansible_host as an empty value like omit or null. Another option would be to treat it as an absent value and remove the line from the config file entirely in that case, but in my opinion that would violate the principle of least surprise.
| line: ansible_host={{ rhc_insights.ansible_host }} | ||
| - name: Update ansible host in inventory | ||
| shell: >- | ||
| insights-client --ansible-host={{ rhc_insights.ansible_host }} & wait |
There was a problem hiding this comment.
When rhc_insights.ansible_host is set to an empty string, this command is run as
insights-client --ansible-host= & waitwhich causes the Ansible hostname value to be reset in the inventory. This may be not what the user intended and exposes behavior specific to the CLI to the RHC role.
We can make this a feature, but in such case we should also remove the ansible_host line from the config file to be consistent. Currently ansible_host= is put in the config file, which is ignored by the Client. This would be best achieved by making the empty string value trigger the remove task rather than this one.
However, I’d rather follow the principle of least astonishment and treat the empty string the same way as other empty values like omit and null. That is to ignore the value and not set or remove anything.
| - name: Remove ansible host | ||
| when: | ||
| - rhc_insights.ansible_host is defined | ||
| - rhc_insights.ansible_host == __rhc_state_absent |
There was a problem hiding this comment.
| - rhc_insights.ansible_host == __rhc_state_absent | |
| - rhc_insights.ansible_host == "" or rhc_insights.ansible_host == __rhc_state_absent |
in case we wanted to use an empty string to remove the value. I don’t prefer this solution however.
|
|
||
| * `null` or an empty value: the ansible host name is not changed. | ||
| * `{state: absent}`: the ansible host name is unset in the insights-client config file and Host Based Inventory (HBI) is updated to use the system host name. | ||
| * any other string value: the ansible host name is changed in Host Based Inventory (HBI). |
There was a problem hiding this comment.
This is not true: an empty string value "" removes the Ansible hostname value from the Inventory. The word “other” suggests that the “empty value” above may refer to an ampty string too, but that’s not true either and yields inconsistent results.
I‘d vote for removing the ambiguous wording “empty value” as value: is equivalent to value: null in YAML. See the conversation above.
| * any other string value: the ansible host name is changed in Host Based Inventory (HBI). | |
| * any non-empty string value: the ansible host name is changed in Host Based Inventory (HBI). |
|
|
||
| Possible values of this variable: | ||
|
|
||
| * `null` or an empty value: the ansible host name is not changed. |
There was a problem hiding this comment.
I found out that an empty string value yields inconsistent results:
ansible_host=is put into the config file, causing the value not get updated on upload.- The Client is called with
--ansible_host=, removing the value from the Inventory.
Even though this “empty value” refers to the YAML syntax value:, it sounds ambiguous to me. I’d rather not describe features of YAML and be more explicit with empty strings.
| * `null` or an empty value: the ansible host name is not changed. | |
| * `null`: the ansible host name is not changed. |
if we wanted to use an empty string to reset the value, which I don’t prefer, or
| * `null` or an empty value: the ansible host name is not changed. | |
| * `null` or an empty string: the ansible host name is not changed. |
if we chose to consider an empty string an empty value, equivalent to null or omit.
It may be worth mentioning omit too.
| * `null` or an empty value: the ansible host name is not changed. | |
| * `null`, `omit`, or an empty string: the ansible host name is not changed. |
| Possible values of this variable: | ||
|
|
||
| * `null` or an empty value: the ansible host name is not changed. | ||
| * `{state: absent}`: the ansible host name is unset in the insights-client config file and Host Based Inventory (HBI) is updated to use the system host name. |
There was a problem hiding this comment.
If we decided that an empty string means the same as {state: absent}:
| * `{state: absent}`: the ansible host name is unset in the insights-client config file and Host Based Inventory (HBI) is updated to use the system host name. | |
| * `{state: absent}` or an empty string: the ansible host name is unset in the insights-client config file and Host Based Inventory (HBI) is updated to use the system host name. |
I don’t prefer this solution though.
There was a problem hiding this comment.
For some history about why we use the seemingly unintuitive {state: absent} - see https://linux-system-roles.github.io/documentation/incremental_settings.html
Also, our usage of omit is problematic - omit should almost never be used except for as a direct argument to a module parameter - users should certainly not be setting role parameters to the value omit - I would prefer not to mention it.
Adds the ansible host name option for insights-client
into the rhc role. This adds support for setting an
ansible host name in inventory during insights-client
registration and updating the current ansible host name
in inventory if the system is already registered.
Changes:
ansible_hostparameter to insights client task to set/update/reset ansible host name in HBI.ansible_hostparameter.Signed-off-by: Jason Jerome jajerome@redhat.com