-
Notifications
You must be signed in to change notification settings - Fork 0
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
Use web app user in logs and process management #208
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #208 +/- ##
=======================================
Coverage ? 82.00%
=======================================
Files ? 31
Lines ? 400
Branches ? 0
=======================================
Hits ? 328
Misses ? 72
Partials ? 0 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. One tiny tweak so that our dummy processes can keep running.
@@ -57,5 +57,5 @@ def form_valid(self, form: BootProcessForm) -> HttpResponse: | |||
Returns: | |||
A redirect to the index page. | |||
""" | |||
boot_process("root", form.cleaned_data) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See above comment on the boot_process
function.
|
||
|
||
async def _boot_process(user: str, data: dict[str, str | int]) -> None: | ||
pmd = get_process_manager_driver() | ||
pmd = get_process_manager_driver(user) | ||
async for item in pmd.dummy_boot(user=user, **data): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One catch is that for any of the dummy processes to run in our docker setup they need to be owned by root, i.e. the user
argument to pmd.dummy_boot
needs to be "root". This is something I need to document properly in #107. We still want the Django user to be named in the kafka messages however so let's move the hard coding of "root" into this function. The hard-coding is ok as this dummy boot function is only a nicety for development (something else we need to document).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Keeping Chris comment aside, this looks good to me!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
"""Get a ProcessManagerDriver instance.""" | ||
token = create_dummy_token_from_uname() | ||
token = Token(token=f"{username}-token", user_name=username) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Assuming this actually works with their authentication system
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the moment they're using a DummyAuthoriser
that doesn't check the token.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good 👍
Description
This PR adresses #111 by passing the web app username - from the
request
object available in views - to function signatures where the username is relevant to the function i.e. logs and process management.Fixes #111
Type of change
Key checklist
python -m pytest
)python -m sphinx -b html docs docs/build
)pre-commit run --all-files
)Further checks