PHACDataHub / django-phac_aspc-helpers

Provides a series of helpers to provide a consistent experience accross PHAC-ASPC's Django based projects.
MIT License
0 stars 0 forks source link

Logging helpers #38

Closed Stephen-ONeil closed 1 year ago

Stephen-ONeil commented 1 year ago

Closes #22

TODO:

github-actions[bot] commented 1 year ago

Coverage

Coverage Report
FileStmtsMissCoverMissing
phac_aspc/django
   excel.py1482881%17–18, 49, 71, 81, 85, 119–121, 125–129, 132–133, 162, 170, 187–188, 214, 217, 219, 221, 224, 230, 236, 255–256, 263–264
   fields.py791680%46, 67–80, 102–105, 120–125, 128–132
phac_aspc/django/admin/decorators
   admin_decorators.py18289%37–38
phac_aspc/django/helpers/auth
   backend.py28280%2–36
   views.py442739%22, 27–30, 35–43, 48–100
phac_aspc/django/helpers/locale
   code.py8362%10, 13–14
phac_aspc/django/helpers/logging
   configure_logging.py29197%187
   json_post_handlers.py27389%8–9, 61
   utils.py11282%43, 48
phac_aspc/django/helpers/templatetags
   phac_aspc_auth.py7186%14
   phac_aspc_localization.py11464%11, 26–29
phac_aspc/django/helpers/views
   wet.py10640%12–19
phac_aspc/django/localization
   hooks.py660%2–16
phac_aspc/django/localization/decorators
   __init__.py220%4–6
   localization_decorators.py11110%4–46
phac_aspc/django/settings
   logging.py18761%28–36, 46–50
   security.py21957%34–45
phac_aspc/django/settings/utils
   configure_settings_for_tests.py3233%15–19
TOTAL67615877% 

Tests Skipped Failures Errors Time
33 0 :zzz: 0 :x: 0 :fire: 2.664s :stopwatch:
Stephen-ONeil commented 1 year ago

I plan to write a few more tests still, just for extra solid coverage, but otherwise this is all ready for review.

Will have an impact on down-stream consumers once released, since the default logging configuration is currently part of the from phac_aspc.django.settings import * pattern, and will require one or two PHAC_ASPC_... env vars depending on a given project's environment. Either I fiddle with that pattern to make it opt-in, or we make this a managed roll-out and walk all of our devs through the new logging feature.

I was also curious, @lucbelliveau, do we have a way to deploy release candidate tagged versions of the package? Could be worth doing that before this merges, because it'd be good to do a pilot test in an Azure Cloud app before this goes wide-release. I believe it's ready, but can't test in Azure myself.

@Dhanani I've tagged you as a reviewer specifically for the small Azure logging handler component. Still think we should meet to get on the same page about logging too!

AlexCLeduc commented 1 year ago

I think this looks good, we'll want to be careful not to interrupt itap with this type of work when merging, the logging seems a bit fragile right now. At the very least I need to get a release out before we merge this

Stephen-ONeil commented 1 year ago

Agreed that it shouldn't be rushed in. I do need at least one project to test the Azure logging capability out since I don't have a playground of my own there. I'll probably revisit putting the whole thing behind an env var flag, disabled by default at least for now.

lucbelliveau commented 1 year ago

@Stephen-ONeil there is no way to deploy RCs to pypi at the moment - but you could try something like this:

 pip install git+https://github.com/PHACDataHub/django-phac_aspc-helpers.git@logging-helpers
Dhanani commented 1 year ago

@Stephen-ONeil For sure! I'm free this Thursday if you want to setup a meeting :)

github-actions[bot] commented 1 year ago

Coverage

Coverage Report
FileStmtsMissCoverMissing
phac_aspc/django
   excel.py1482881%17–18, 49, 71, 81, 85, 119–121, 125–129, 132–133, 162, 170, 187–188, 214, 217, 219, 221, 224, 230, 236, 255–256, 263–264
   fields.py791680%46, 67–80, 102–105, 120–125, 128–132
phac_aspc/django/admin/decorators
   admin_decorators.py18289%37–38
phac_aspc/django/helpers
   jinja_dtl_interop_utils.py7186%21
phac_aspc/django/helpers/auth
   backend.py28280%2–36
   views.py442739%22, 27–30, 35–43, 48–100
phac_aspc/django/helpers/locale
   code.py8362%10, 13–14
phac_aspc/django/helpers/logging
   configure_logging.py29197%187
   json_post_handlers.py27389%8–9, 61
   utils.py11282%43, 48
phac_aspc/django/helpers/templatetags
   phac_aspc_auth.py7186%14
   phac_aspc_localization.py11464%11, 26–29
phac_aspc/django/helpers/views
   wet.py10640%12–19
phac_aspc/django/localization
   hooks.py660%2–16
phac_aspc/django/localization/decorators
   __init__.py220%4–6
   localization_decorators.py11110%4–46
phac_aspc/django/settings
   logging.py18761%28–36, 46–50
   security.py21957%34–45
phac_aspc/django/settings/utils
   configure_settings_for_tests.py3233%15–19
TOTAL70315977% 

Tests Skipped Failures Errors Time
37 0 :zzz: 0 :x: 0 :fire: 2.824s :stopwatch:
Stephen-ONeil commented 1 year ago

Oop, that last second merge from main bumped the linter version on this branch and added a new lint error. I'll hotfix that over on main.