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 #37474 - feat: enable updating ansible override values via api #719

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

gardar
Copy link
Contributor

@gardar gardar commented May 15, 2024

@gardar gardar changed the title feat: enable updating ansible override values via api fixes #37474 - feat: enable updating ansible override values via api May 15, 2024
@gardar gardar force-pushed the feat/update_ansible_override_values branch from ce46349 to e3f4c01 Compare May 15, 2024 20:04
@gardar gardar force-pushed the feat/update_ansible_override_values branch from e3f4c01 to dbdcf3a Compare May 15, 2024 20:14
@gardar gardar marked this pull request as draft May 15, 2024 20:37
@gardar gardar force-pushed the feat/update_ansible_override_values branch from dbdcf3a to d8176a0 Compare May 16, 2024 11:24
@gardar gardar marked this pull request as ready for review May 16, 2024 11:24
@nofaralfasi nofaralfasi self-assigned this May 21, 2024
Copy link
Contributor

@nofaralfasi nofaralfasi left a comment

Choose a reason for hiding this comment

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

I noticed two things:

  1. When trying to update the value for a variable that doesn't have an override value, we get an error. We should catch that error and print an informative message to the user.
  2. When a user without the required permissions tries to override a value, the necessary permissions are not specified in the error message.

@gardar
Copy link
Contributor Author

gardar commented Jun 6, 2024

I noticed two things:

1. When trying to update the value for a variable that doesn't have an override value, we get an error. We should catch that error and print an informative message to the user.

I was actually originally planning to modify the current POST method of the /ansible_override_values so that in addition to creating new overrides it would update existing ones. So rather than using POST for creating and PUT for updating the POST would be used for both. I would much prefer that approach myself but I decided to follow the design of the other api endpoints which use POST for creating and PUT for updating.
What are your thoughts? Can we use POST for both? Then we don't have to print a informative message to the user.

2. When a user without the required permissions tries to override a value, the necessary permissions are not specified in the error message.

In my experience this is actually the case for the other api endpoints as well, the necessary permissions are not specified in the error message.
Perhaps all the api endpoints should be updated with that regard in a separate PR?

@nofaralfasi
Copy link
Contributor

I was actually originally planning to modify the current POST method of the /ansible_override_values so that in addition to creating new overrides it would update existing ones. So rather than using POST for creating and PUT for updating the POST would be used for both. I would much prefer that approach myself but I decided to follow the design of the other api endpoints which use POST for creating and PUT for updating. What are your thoughts? Can we use POST for both? Then we don't have to print a informative message to the user.

Why should we use POST for both? I think we should stick to the expected behavior where POST is for creating and PUT is for updating.

In my experience this is actually the case for the other api endpoints as well, the necessary permissions are not specified in the error message. Perhaps all the api endpoints should be updated with that regard in a separate PR?

The corresponding POST request for creating the override value specifies the necessary permissions in the error message. We can follow the same functionality for the PUT request as well.

@gardar gardar force-pushed the feat/update_ansible_override_values branch from a893b89 to cbd71d6 Compare August 8, 2024 15:58
@gardar gardar requested a review from nofaralfasi August 8, 2024 15:58
config/routes.rb Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants