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.

Sign up for GitHub

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

Jump to bottom

Add AnsibleVariable to entities #1220

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

Add AnsibleVariable to entities #1220

wants to merge 1 commit into 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?

@dosas dosas force-pushed the add-ansible-variables-api branch from 3353ca4 to 3fbcc5c Compare September 11, 2024 12:57
@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
Add support for Ansible roles and Ansible variables #753
Open
@dosas
Copy link
Collaborator Author

dosas commented Oct 7, 2024 •
edited
Loading

@Gauravtalreja1 Would you like to review this PR?

vsedmik
vsedmik previously approved these changes Nov 13, 2024
View reviewed changes
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 dosas force-pushed the add-ansible-variables-api branch from 3fbcc5c to bec86cb Compare November 13, 2024 15:46
@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.

Gauravtalreja1
Gauravtalreja1 reviewed Nov 13, 2024
View reviewed changes
nailgun/entities.py Outdated
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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

IMHO this is a bug theforeman/foreman_ansible#744

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@Gauravtalreja1 @vsedmik As suggested by you above I would merge and cherry-pick the changes and then add the hidden value field to stream once the bug is fixed.

nailgun/entities.py Outdated Show resolved Hide resolved
nailgun/entities.py Outdated Show resolved Hide resolved
@dosas dosas force-pushed the add-ansible-variables-api branch from bec86cb to e66be96 Compare November 14, 2024 12:50
@dosas dosas mentioned this pull request Nov 26, 2024
Open
1 task
@dosas dosas force-pushed the add-ansible-variables-api branch from e66be96 to 7cc3b5a Compare December 9, 2024 12:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vsedmik vsedmik Awaiting requested review from vsedmik

@Gauravtalreja1 Gauravtalreja1 Awaiting requested review from Gauravtalreja1

At least 2 approving reviews are required to merge this pull request.

Assignees

@dosas dosas

Labels
6.13.z 6.14.z 6.15.z 6.16.z CherryPick PR needs CherryPick to previous branches
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add support for Ansible roles and Ansible variables
4 participants
@dosas @omkarkhatavkar @vsedmik @Gauravtalreja1