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

[Fix niteoweb/pyramid_heroku#22] Add request_id... #38

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

Conversation

sourya
Copy link

@sourya sourya commented Sep 10, 2019

... to sentry and structlog logs

Signed-off-by: Sourya Vatsyayan [email protected]

... to sentry and structlog logs

Signed-off-by: Sourya Vatsyayan <[email protected]>
def __call__(self, request):
request_id = request.headers.get("X-Request-ID")

if request_id is not None:
Copy link
Member

Choose a reason for hiding this comment

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

Less nesting is better. So it's better to have

if not request_id:
    return self.handler(request)
... business logic here ...

Copy link
Author

Choose a reason for hiding this comment

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

Done.

import sentry_sdk
IS_SENTRY_ENABLED = True
except ImportError:
IS_SENTRY_ENABLED = False
Copy link
Member

Choose a reason for hiding this comment

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

This should be SENTRY_INSTALLED, not "enabled".

Copy link
Author

Choose a reason for hiding this comment

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

Done.

@@ -0,0 +1,9 @@
import unittest

from .test_herokuapp_access import TestHerokuappAccessTween
Copy link
Member

@zupo zupo Sep 10, 2019

Choose a reason for hiding this comment

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

Please use absolute, not relative, imports. Easier to sort, and to move around, fever problems with circular dependencies, easier for IDEs to parse, etc.

Copy link
Author

Choose a reason for hiding this comment

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

Removed this file, since pytest discovers the test files. I was trying to do it with python -m unittest pyramid_heroku.tests.

@zupo
Copy link
Member

zupo commented Sep 10, 2019

Please use this branch of pyramid_heroku in a Review App of one of the apps so that we confirm things are working in a production-like environment.

Mocking module imports doesn't go along well with pytest.
sys.modules doesn't remain the same throughout. Besides,
the previous implementation used global vars in tests, which
is not really recommended.

Signed-off-by: Sourya Vatsyayan <[email protected]>
This brings up test coverage to 100%. B-)
Also, add type hints in some functions and the global vars.

Signed-off-by: Sourya Vatsyayan <[email protected]>
@sourya
Copy link
Author

sourya commented Sep 12, 2019

I don't quite understand the workflow of a review app. Is there an existing Pyramid app that I can fork to test and deploy, or should I build my own from scratch?

EDIT:

Using https://github.com/niteoweb/pyramid-realworld-example-app

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.

3 participants