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

Remove flask dependency api layer #373

Merged
merged 1 commit into from
Mar 9, 2018
Merged

Conversation

tbille
Copy link
Contributor

@tbille tbille commented Mar 9, 2018

Refactoto S02E01 - The first Endpoint

Summary

  • Remove flask dependency from api public layer.
  • First draft for a better error handler in the api layer
  • Work on public snap access

QA

  • ./run
  • ./run test
  • 0.0.0.0:8004/toto
  • Should have a snap
  • 0.0.0.0:8004/qsdfsqdfqsdf
  • Should return 404 page

@webteam-app
Copy link

Starting demo at: http://snapcraft.io-pr-373.run.demo.haus/

Copy link
Contributor

@Lukewh Lukewh left a comment

Choose a reason for hiding this comment

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

Yay Season 2 👍 thanks @tbille

@tbille tbille merged commit 50faea2 into canonical:master Mar 9, 2018
@tbille tbille deleted the refactor-s02e01 branch March 9, 2018 09:21
Copy link
Contributor

@cprov cprov left a comment

Choose a reason for hiding this comment

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

some comments, post-merge :-/

"JSON decoding failed: ",
str(decode_error),
])
raise InvalidResponseContent(error_message)
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpicking on style, the string-joining looks strange:

error_message = 'JSON decoding failed: {}'.format(decode_error)

])
raise InvalidResponseContent(error_message)

if details_response.status_code != 200:
Copy link
Contributor

Choose a reason for hiding this comment

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

The store API uses other 2xx status and they should be treated as success.


if details_response.status_code != 200:
if 'error_list' in details:
api_error_exception = ApiErrorResponse("Error list")
Copy link
Contributor

Choose a reason for hiding this comment

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

You might get some expiration from https://git.launchpad.net/snapdevicegw/tree/snapdevicegw/exceptions.py and have a chain of exceptions that would treat the API errors internally, APIError(error_response)

if api_error_exception.errors:
flask.abort(api_error_exception.status, api_error_exception.errors)
else:
flask.abort(api_error_exception.status, ["Unknown error"])
Copy link
Contributor

Choose a reason for hiding this comment

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

It doesn't seem to be a good idea to blindly 'leak' snap-details error status to you own application, nor leaking bad-json responses to ugly 500s to the app users. Wouldn't it be possible to tolerate errors and carry on with details = {...} ?

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.

None yet

4 participants