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

Add AnsibleVariable to entities #1220

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

dosas
Copy link
Collaborator

@dosas dosas commented Aug 22, 2024

Description of changes

Add AnsibleVariable to entities

Upstream API documentation, plugin, or feature links

I would like to reference to api doc but the latest version I can find is 3.4 ...

Functional demonstration

Example:

module_target_sat.api.AnsibleVariable(
            variable="foreman_cloud_connector_user",
            default_value="foo",
            ansible_role_id=ansible_role_id,
        ).create()
robottelo.hosts.DecClass(variable='foreman_cloud_connector_user', ansible_role_id=24, default_value='foo', id=54)

@dosas dosas added CherryPick PR needs CherryPick to previous branches 6.15.z 6.16.z labels Aug 22, 2024
@omkarkhatavkar
Copy link

Can one of the admins verify this patch?

@ogajduse ogajduse requested a review from a team October 3, 2024 19:49
@ogajduse ogajduse linked an issue Oct 3, 2024 that may be closed by this pull request
@dosas
Copy link
Collaborator Author

dosas commented Oct 7, 2024

@Gauravtalreja1 Would you like to review this PR?

vsedmik
vsedmik previously approved these changes Nov 13, 2024
Copy link
Contributor

@vsedmik vsedmik left a comment

Choose a reason for hiding this comment

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

Looks good to me. Matches the apidoc and works locally too.

@dosas
Copy link
Collaborator Author

dosas commented Nov 13, 2024

@vsedmik thank you for the review, do you know somebody who could supply the second review?

@vsedmik
Copy link
Contributor

vsedmik commented Nov 13, 2024

@vsedmik thank you for the review, do you know somebody who could supply the second review?

@SatelliteQE/team-rocket should be in charge. Let me poke them offline.

Comment on lines 9232 to 9233
'default_value': entity_fields.StringField(),
'override_value_order': entity_fields.StringField(),
Copy link
Collaborator

Choose a reason for hiding this comment

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

According to the APIdoc, only hidden_value field is missing here, could you add it?
https://apidocs.theforeman.org/katello/latest/apidoc/v2/ansible_variables/update.html

Copy link
Contributor

Choose a reason for hiding this comment

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

There is some diff between API and apidoc regarding the hidden_value. Apidoc says "ansible_variable[hidden_value]" but in response I can see we get hidden_value?.

2024-11-14 09:49:59 - nailgun.client - DEBUG - Received HTTP 200 response: {"parameter":"var_with_hidden_val","id":75,"variable":"var_with_hidden_val","ansible_role":"RedHatInsights.insights-client","ansible_role_id":1,"description":"","override":true,"variable_type":"string","hidden_value?":true,"validator_type":"","validator_rule":null,"merge_overrides":false,"merge_default":false,"avoid_duplicates":false,"override_value_order":"fqdn\nhostgroup\nos\ndomain","created_at":"2024-11-13 13:58:13 UTC","updated_at":"2024-11-13 13:58:13 UTC","default_value":"abc","imported":false,"override_values":[],"override_values_count":0}

So the change needed would be this, which looks quite weird

Suggested change
'default_value': entity_fields.StringField(),
'override_value_order': entity_fields.StringField(),
'default_value': entity_fields.StringField(),
'override_value_order': entity_fields.StringField(),
'hidden_value?': entity_fields.BooleanField(),

@Gauravtalreja1 Can you check with DEVs what is expected and if they need to fix the API or apidoc?

Anyway, I wouldn't block the PR until it's resolved. It can be added anytime when needed.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Seeing that the PR has been open for ~ 3 months I would not start to hurry now and clarify this in that PR.

nailgun/entities.py Outdated Show resolved Hide resolved
nailgun/entities.py Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.13.z 6.14.z 6.15.z 6.16.z CherryPick PR needs CherryPick to previous branches
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for Ansible roles and Ansible variables
4 participants