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

Multiple fixes for new backend #49

Merged
merged 6 commits into from
Oct 3, 2023

Conversation

dylanmcreynolds
Copy link
Member

@dylanmcreynolds dylanmcreynolds commented Sep 29, 2023

Some small fixes for issues found testing against the new backend:

  • The new backend reports errors directly in the message body, while the old backed used an "err" object in the json.
  • The new backend reports status and errors differently from the old backend, this accommodates those changes. One major change is that there was an inconsistency in reporting 404. Because of this, @jl-wynen added a mechanism in the client code to specify that operations not throw errors if the return is a 404. This PR removes that because as far as I can tell, the new backend only reports 404s when the URL path is bad, in which case throwing a ScicatCommError is desirable.

@dylanmcreynolds dylanmcreynolds changed the title Small fixes for new backend Multiple fixes for new backend Sep 29, 2023
Copy link
Collaborator

@jl-wynen jl-wynen left a comment

Choose a reason for hiding this comment

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

👍 on simplifying this

Copy link
Collaborator

Choose a reason for hiding this comment

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

You missed the error handling here

f'Error retrieving token for user: {err["name"]}, {err["statusCode"]}: {err["message"]}'
and here
f' Failed log in: {err["name"]}, {err["statusCode"]}: {err["message"]}'

Also, I noticed that Client._log_in_via_auth_msad doesn't return anything, this is probably on me. Is this method still used by anyone?

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for the catch. I just pushed a commit for that.

I don't know if anyone uses that method, but the server supports the endpoint, so I think we have to leave it in.

@dylanmcreynolds dylanmcreynolds merged commit 67b0556 into SciCatProject:main Oct 3, 2023
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