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

RANGER-3370: Python ranger_client call_api - add case for 404 response #110

Closed
wants to merge 6 commits into from

Conversation

alvaroqueiroz
Copy link
Contributor

@alvaroqueiroz alvaroqueiroz commented Jul 17, 2021

One exemple of problem this PR wants to solve:
When using the method ranger.get_policy(), if the policy do not exists, i get the following error:

image

It will crash any program.

This will happen with other methods too.

It happens because the API call do not have a case for dealing with the response 404 (resource do not exist).
So i'm adding it

After this fix, when ranger.get_policy() method is called for a policy da does not exists, it will return None

@alvaroqueiroz
Copy link
Contributor Author

alvaroqueiroz commented Jul 17, 2021

@chia7712 Can you help me with this CI error? :)

@chia7712
Copy link
Contributor

Can you help me with this CI error? :)

I filed a PR (#105) to fix CI. However, I'm not Ranger committer so it needs love from other guys :)

@alvaroqueiroz
Copy link
Contributor Author

@mneethiraj can you take a look at this PR?

@alvaroqueiroz
Copy link
Contributor Author

@chia7712 thank you for your fast response, i will try to get the attention of the comitters

@alvaroqueiroz
Copy link
Contributor Author

alvaroqueiroz commented Jul 22, 2021

@kulkabhay @coheigea @rameeshm @gautamborad @zhangqiang2 @sneethiraj
Please, can any of you review this PR? i need use it in my application, and @mneethiraj do not repond :(

@alvaroqueiroz
Copy link
Contributor Author

@chia7712 my friend, do you know any body that can look at my PR? hahaha

I mean, is there any active ranger committer i can call?

@chia7712
Copy link
Contributor

I mean, is there any active ranger committer i can call?

Sorry that I'm newbie in ranger community :(

@@ -324,6 +324,11 @@ def __call_api(self, api, query_params=None, request_data=None):
LOG.error("Ranger admin unavailable. HTTP Status: %s", HTTPStatus.SERVICE_UNAVAILABLE)

ret = None
elif response.status_code == HTTPStatus.SERVICE_NOT_FOUND:
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider renaming SERVICE_NOT_FOUND => NOT_FOUND.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks for the comments @mneethiraj, it makes sense

@@ -324,6 +324,11 @@ def __call_api(self, api, query_params=None, request_data=None):
LOG.error("Ranger admin unavailable. HTTP Status: %s", HTTPStatus.SERVICE_UNAVAILABLE)

ret = None
elif response.status_code == HTTPStatus.SERVICE_NOT_FOUND:
LOG.error("Ranger service not found. HTTP Status: %s", HTTPStatus.SERVICE_NOT_FOUND)
Copy link
Contributor

Choose a reason for hiding this comment

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

"Ranger service not found" => "Not found"

@alvaroqueiroz
Copy link
Contributor Author

@mneethiraj thank you for your comments, the changes make sense.

@alvaroqueiroz
Copy link
Contributor Author

@mneethiraj is everything fine?

@alvaroqueiroz
Copy link
Contributor Author

@mneethiraj ?

@alvaroqueiroz
Copy link
Contributor Author

@mneethiraj can you please take a look?

@alvaroqueiroz
Copy link
Contributor Author

@mneethiraj ?

@mneethiraj
Copy link
Contributor

@alvaroqueiroz - thank you for the patch with the updates. I filed RANGER-3370, and merged this patch in master and ranger-2.2 branches.

@mneethiraj mneethiraj closed this Aug 13, 2021
@mneethiraj mneethiraj changed the title Python ranger_client call_api - add case for 404 response RANGER-3370: Python ranger_client call_api - add case for 404 response Aug 13, 2021
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.

5 participants