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 #37417 - OverriddenAnsibleVariablePresenter GraphQL scheme incorrect for hidden_value and override #716

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

Conversation

Thorben-D
Copy link
Contributor

@Thorben-D Thorben-D commented May 8, 2024

In the model AnsibleVariable, there is hidden_value, which returns "*****" and there is hidden_value**?**, which returns a boolean.
For GraphQL, the method name must be the same as the value entered in the gql-query. By delegating hidden_value**?**, the original author effectively named the gql-field "hidden_value?", which is not allowed and was therefore not working.
The same applies to override.

This is fixed by wrapping the value of hidden_value**?** in a new method hidden_value in the presenter.

Required by #717

@Thorben-D Thorben-D force-pushed the fixes/37417_overridden_ansible_variable_presenter_gql_fix branch 2 times, most recently from 500fe9e to b8a6df8 Compare May 8, 2024 12:45
@sbernhard
Copy link
Contributor

@stejskalleos can you please review this?

@nofaralfasi
Copy link
Contributor

I'm unclear on what exactly isn't working and what we're trying to fix with these changes. Additionally, I'm not sure where the new methods are being used.

@Thorben-D
Copy link
Contributor Author

In the AnsibleVariable model, hidden_value returns a series of asterisks, while hidden_value? returns a boolean. The author's intention was to delegate hidden_value?, but this led to an illegal field name "hidden_value?" in the GraphQL query. Similarly, override? faced the same issue. This has been rectified by wrapping hidden_value? in a new method named hidden_value within the presenter.

@nofaralfasi
Copy link
Contributor

In the AnsibleVariable model, hidden_value returns a series of asterisks, while hidden_value? returns a boolean. The author's intention was to delegate hidden_value?, but this led to an illegal field name "hidden_value?" in the GraphQL query. Similarly, override? faced the same issue. This has been rectified by wrapping hidden_value? in a new method named hidden_value within the presenter.

The override and hidden_value fields are defined in the AnsibleVariable type, while the OverridenAnsibleVariablePresenter acts as a bridge between the raw Ansible variable data and the GraphQL API, ensuring that values are properly resolved and presented to the client. However, in the current version of the hostVariableOverrides query, neither override nor hidden_value are included. The exclusion of override seems intentional due to its straightforward nature, but the hidden_value field could be incorporated into the query with the correct logic.

@Thorben-D
Copy link
Contributor Author

You're right, right now neither hidden_value nor override is queried. This PR is a prerequisite to #717, which needs the hidden_value field via the gql-query.
I should have linked those bi-directionally...

@sbernhard
Copy link
Contributor

Can we somehow continue this PR?

We had the following issue, which we were able to fix by this PR.
==> /var/log/foreman/production.log <== [I|app|ac0ec7bb] Started POST "/api/graphql" for 10.1.90.35 at 2024-11-02 04:15:18 +0100 [I|app|ac0ec7bb] Processing by Api::GraphqlController#execute as JSON [I|app|ac0ec7bb] Parameters: {"_json"=>"[FILTERED]", "graphql"=>{"_json"=>"[FILTERED]"}} [W|app|ac0ec7bb] Action failed [I|app|ac0ec7bb] Backtrace for 'Action failed' error (ArgumentError): wrong number of arguments (given 4, expected 0) ac0ec7bb | /usr/share/gems/gems/graphql-1.13.21/lib/graphql/execution/errors.rb:119:inmessage'
ac0ec7bb | /usr/share/gems/gems/graphql-1.13.21/lib/graphql/execution/errors.rb:119:in rescue in with_error_handling' ac0ec7bb | /usr/share/gems/gems/graphql-1.13.21/lib/graphql/execution/errors.rb:106:in with_error_handling'
ac0ec7bb | /usr/share/gems/gems/graphql-1.13.21/lib/graphql/query.rb:365:in with_error_handling'

I don't see a reason why such a PR is not merged. If there is a request which should be adapted, please let us exactly know so that we can finish it.

@nofaralfasi
Copy link
Contributor

Hi @sbernhard, I thought this PR is needed for #717, which is not ready yet (see my comment here https://github.com/theforeman/foreman_ansible/pull/717/files#r1741039438).
If this PR is intended to fix a different issue, could you please describe it and add steps to reproduce it?

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

Successfully merging this pull request may close these issues.

3 participants