Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes #36136 - handle the case where Ansible variable is an array #649

Merged
merged 1 commit into from Aug 3, 2023

Conversation

nofaralfasi
Copy link
Contributor

No description provided.

@nofaralfasi
Copy link
Contributor Author

nofaralfasi commented Jul 12, 2023

In cases where we have more than 1 matcher, the UI would look like this:
image

Details of the Ansible variable:
image

Do we want to modify it to improve clarity?

@MariaAga
Copy link
Member

@nofaralfasi How did it look before the change? Should we also change how host parameters behave? (the code in host parameters is similar to code of ansible variables, we should probably do some merging between them when we have the time)

@nofaralfasi
Copy link
Contributor Author

@nofaralfasi How did it look before the change?

It looked exactly the same before my changes.

Should we also change how host parameters behave? (the code in host parameters is similar to code of ansible variables, we should probably do some merging between them when we have the time)

AFAIK, the host parameters code is different since we don't have the option to override the parameters using Matchers, as we do with Ansible variables. Correct me if I'm wrong here.

@MariaAga
Copy link
Member

@nofaralfasi Didnt notice that variables have an option to merge overrides, and the parameters dont. So it should be fine as only a fix in Ansible :)

@stejskalleos stejskalleos self-assigned this Jul 13, 2023
@MariSvirik
Copy link

We can either use slash as you proposed for editing mode OR we can even add an extra field for each merged value under the first one when in editing mode. That might be visually easier to distinguish if the users want to edit the thing from this page.
I think I had in my designs a tooltip saying that merging overrides are allowed. We should definitely display the information somewhere e.g. a tooltip or in the source attribute column.

@stejskalleos
Copy link
Contributor

@nofaralfasi
I'm not sure what are we fixing here. Displaying variable on host detail or editing matchers?

@nofaralfasi
Copy link
Contributor Author

@nofaralfasi I'm not sure what are we fixing here. Displaying variable on host detail or editing matches?

This PR is for fixing the "TypeError: string.split is not a function" error when accessing ansible variables on a host.

I took the opportunity to ask @MariSvirik's opinion regarding the Ansible variables UI. In my opinion, there is room for improvement in terms of how we present the information, as it may not be sufficiently clear.
To continue this discussion, I will create a community post since it is unrelated to the current issue at hand.

@stejskalleos
Copy link
Contributor

This PR is for fixing the "TypeError: string.split is not a function" error when accessing ansible variables on a host.

How do I reproduce the issue then? I have Foreman + Ansible + Theme plugin and works fine for me. Tried two ansible variables, one string, second array, and the page is loading fine. Any concrete steps on how to reproduce it?

To continue this discussion, I will create a community post since it is unrelated to the current issue at hand.

+1, let's not mix it here.

@nofaralfasi
Copy link
Contributor Author

How do I reproduce the issue then? I have Foreman + Ansible + Theme plugin and works fine for me. Tried two ansible variables, one string, second array, and the page is loading fine. Any concrete steps on how to reproduce it?

  1. Create a new Ansible variable with an array type. Default value is ["test"]
  2. Enable both 'Merge Overrides' and 'Merge Default'
  3. Add the variable to a role
  4. Attach the role to a host
  5. Check the ansible variables on the host details page

Copy link
Contributor

@stejskalleos stejskalleos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🍏 LGTM
it works and fix the issue

@stejskalleos stejskalleos merged commit cea1eca into theforeman:master Aug 3, 2023
7 checks passed
@stejskalleos
Copy link
Contributor

Thanks @nofaralfasi

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants