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

18 flask 1x upgrade #455

Merged
merged 7 commits into from
Sep 10, 2018
Merged

18 flask 1x upgrade #455

merged 7 commits into from
Sep 10, 2018

Conversation

benvand
Copy link
Contributor

@benvand benvand commented Sep 7, 2018

http://flask.pocoo.org/docs/1.0/changelog/#version-1-0
https://trello.com/c/bBDir2Gp/18-flask-1x-upgrade

  • Prevent overwriting of imported logger module in dmutils/logging
  • New fixture to stream logs to StreamIO object (replaces log_to_file method)
  • New fixture to mock logger attribute of app
  • Changes to look for 'flask.app' new logger name
  • Break out some parametrized tests to reduce complexity
  • Use new fixtures where possible
  • Add new autouse fixture to prevent stdout logging in tests

@benvand benvand force-pushed the 18-flask-1x-upgrade branch 2 times, most recently from 365daf5 to ee08832 Compare September 7, 2018 11:07
@lfdebrux lfdebrux self-requested a review September 7, 2018 11:25
@benvand benvand force-pushed the 18-flask-1x-upgrade branch 2 times, most recently from efb7485 to 371fee7 Compare September 7, 2018 13:49
http://flask.pocoo.org/docs/1.0/changelog/#version-1-0
https://trello.com/c/bBDir2Gp/18-flask-1x-upgrade

* Prevent overwriting of imported `logger` module in dmutils/logging
* New fixture to stream logs to StreamIO object (replaces `log_to_file` method)
* New fixture to mock `logger` attribute of `app`
* Changes to look for 'flask.app' new logger name
 * http://flask.pocoo.org/docs/1.0/changelog/#version-1-0
* Break out some parametrized tests to reduce complexity
* Use new fixtures where possible
@benvand benvand force-pushed the 18-flask-1x-upgrade branch from 371fee7 to d8e4490 Compare September 7, 2018 13:50
@benvand benvand force-pushed the 18-flask-1x-upgrade branch from 374b492 to f33fa0c Compare September 10, 2018 08:06
if raise_exception is not None:
raise raise_exception("Boo")

stream.seek(0)
Copy link
Contributor

Choose a reason for hiding this comment

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

Though it doesn't directly matter here, I think it's probably a better habit to use StringIO's .getvalue() method for this rather than reaching in and seeking around the file. If someone were to make an assertion like this and leave the (imaginary) file descriptor not at the end of the file & continue on with a couple of extra steps of the test, you'd get some confusing mixed up output and it wouldn't be clear why.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I tried that but it meant that I had to mess with the comprehension below quite a lot and I wanted to leave it as similar to before as possible. get_value returns the whole string joined by linebreaks 😒

Copy link
Contributor

Choose a reason for hiding this comment

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

😕 Isn't that exactly what stream.read() does?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call, can use getvalue and remove the seek line 💯

from boto.ec2.cloudwatch import CloudWatchConnection

from dmutils.logging import init_app


@pytest.fixture(scope='session', autouse=True)
def log_to_null():
Copy link
Contributor

Choose a reason for hiding this comment

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

We should consider moving some of these to -test-utils in course.

Copy link
Contributor

@risicle risicle left a comment

Choose a reason for hiding this comment

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

WFM

benvand and others added 4 commits September 10, 2018 13:02
Breaking change.

Although Mandrill is deprecated and only used in one app these days, I
refactored it because it is still required in one app
(supplier-frontend) and was a pain to use.

This commit makes it more in line with the DMNotifyClient, by making the
client wrapper a class, and also it makes the order of arguments to
send_email a bit more intuitive (IMO).
Since we are customising the NotifyClient for our own purposes, we might
as well make it fit our systems a bit better.

This commit makes a bunch of changes to make using DMNotifyClient a bit
less annoying.

You don't have to use these features, but they might make your life
better ;)

- Automatically gets the API key from the current app config

- Looks up the template uuid from the current app config

- Reorder some method arguments and make some arguments
  keyword-only to emphasise that their use should be avoided

- Improve logging of DMNotifyClient so now additional logging
  can be more to the point

  - Logging output has been drastically simplified, details are
    still in JSON extra object however.

  - Removed get_error_message method as it was not adding value
    and was not being used.
@benvand benvand force-pushed the 18-flask-1x-upgrade branch from 74f2325 to 6951a12 Compare September 10, 2018 13:06
@benvand benvand merged commit bcf18a3 into master Sep 10, 2018
@benvand benvand deleted the 18-flask-1x-upgrade branch September 10, 2018 13:37
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