feat: Add a display name parameter #166
Conversation
richm
changed the title
Glutexo
force-pushed
the
display_name
branch
from
71b99df
to
1b15dd2
Compare
|
Fixed the typos. Going to add the tests. |
|
I added the tests, so I am opening the pull request for a review. |
There was a problem hiding this comment.
I had a quick look over -- make sure the tests are referencing display_name instead of ansible_host. I know the tests are near-identical to ansible_host tests but I'm assuming leaving the fields as ansible_host was unintentional.
(Edit: see below for specific lines highlighted by richm)
I can test these changes shortly after you push the changes 👍
There was a problem hiding this comment.
{state: absent}: the display name is unset in the insights-client config file; please note that Host Based Inventory is not updated
I don't think this is a good approach: the user will see that the insights-client configuration was changed (good), but not what is in Inventory (bad), leading the users to think that their operation succeeded. If it is not possible to "unset" (i.e. reset to the hostname) the display name, then the role ought to not allow the users to do this.
Hence:
- drop the handling of
{state: absent}altogether - handle empty values as
null, documenting this behaviour as limitation in the underlying tooling (insights-client)
|
I spotted an issue when
It is the same problem as I found in #155 (review). Since this pull request already has an approval from Jason, it may make sense to merge it as it is and fix the bug for both parameters at the same time in a follow-up pull request. If you think it’s worth including in this one, I‘ll push the fix. |
|
The Moreover, the non-ideal state that may cause confusion to the user (although the real behavior is documented) would only last until the Client is patched to support display name removal. That is true for the absence (😃) of Still, I’d be more inclined to offer a way to clean the configuration, when there is one to add it. What do you think, Pino? |
I'm not sure what would be the point of removing the value from the config file, if that will not reflect what is the state in Inventory. Please see my previous comment which explains the situation as I see it: #166 (review)
Yes, and I do not think the role ought to do operations that do not actually work. When |
|
Since Pino requested changes on the pull request, I believe I can fix the empty string issue in this pull request. Pushed a new commit that causes the empty string to be treated as an empty value: the display name is ignored in that case. |
|
I think I understand. The role is not primarily intended to edit the Client config file. It uses the Client as a tool to perform changes in the Inventory and changes the config file so the configuration is consistent with the new state and doesn’t get reverted on the next collection. In that case, removing |
|
Pushed a patch removing the |
Yes and no. Take a step back: a role/playbook/module/etc is nothing more than a glorified/structured way to do operations and check their results. Whatever is done with Ansible, it can be run manually to get the same result. In case of this role, its purpose is to create and maintain the connectivity of systems to Red Hat or Satellite, Insights included. If to do so some configuration files must be edited, then they are edited; if some command must be run, they are run; and so on. In the specific case of this option, we need to ensure that |
There was a problem hiding this comment.
The changes seem fine to me now, thanks!
The only thing to check would be what Rich mentioned (and I added some notes) on the documentation about the lack of "unset"; let's include what I wrote (or something along those lines), if that looks OF for you.
Since the time for merging is closer, please rebase and squash the commits into one with a proper commit message for it.
|
I added a note on the default display name value to the documentation. I found the suggested wording a bit too verbose, so I tried to shorten it a bit. |
this still needs to be done |
Glutexo
force-pushed
the
display_name
branch
from
0324dca
to
e5a8ccb
Compare
|
Squashed the branch into a single commit and force-pushed. |
|
[citest] |
Glutexo
force-pushed
the
display_name
branch
from
e5a8ccb
to
b78e932
Compare
The new rhc.display_name parameter allows to set a display name, a
value used to identify the host in the Host-Based Inventory. Unlike
Ansible host name, display name can’t be reset to default by Insights
Client, thus the {state: absent} value is not supported.
|
I realized that I added the empty string condition to fix a buggy behavior, but without a test. I added a test verifying that setting rhc_insights.display_name to an empty string doesn’t change the value in the config file, just as a null doesn’t. Without the added condition, the test would fail, because the empty string would change the line in the config file to I didn’t test calling the |
|
[citest] |
|
From what I found that the Insights Client doesn’t send an empty display name, I am a bit afraid that adding support for the Client to reset the value in the Inventory would require a change on the Platform side. I will however verify, what is the actual behavior of sending various empty values of the display name. |
Enhancement:
A new rhc_insights.display_name parameter.
Reason:
The display name is a value managed by Insights Client. It is visible in the Host-Based Inventory and is used as the main visible name in the UI to identify a host and search for it.
Result:
The new rhc_insights.display_name parameter sets the display name of the host. This value is written to the Insights Client config file and also set directly by a
--display-namecommand. When marked as absent or on unregistration, the value is removed from the config file.Issue Tracker Tickets (Jira or BZ if any):
https://issues.redhat.com/browse/CCT-13