-
Notifications
You must be signed in to change notification settings - Fork 1
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
Bug/WI-67: Logging and forcing tapis token retrieval on every request #6
Conversation
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
name: Build on push to bug/WI-67-connection-issues | ||
on: | ||
push: | ||
branches: [ bug/WI-67-connection-issues ] |
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.
Should we maybe abstract this and create a "experiment" branch that we push changes like this to, instead of creating workflows for each unusual case like this?
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.
Yes, I agree.
|
||
@app.before_request | ||
def before_request_log(): | ||
app.logger.info(f"{request.remote_addr} \"{request.method} {request.url}\"") |
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.
If the logging level is set to DEBUG shouldn't it be:
app.logger.info(f"{request.remote_addr} \"{request.method} {request.url}\"") | |
app.logger.debug(f"{request.remote_addr} \"{request.method} {request.url}\"") |
If we just want this log at the info level and available all the time in the logs, I'm fine with that too.
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.
Agree, will move it to DEBUG.
|
||
@app.after_request | ||
def after_request_log(response): | ||
app.logger.info(f"{request.remote_addr} \"{request.method} {request.url}\" {response.status_code}") |
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.
app.logger.info(f"{request.remote_addr} \"{request.method} {request.url}\" {response.status_code}") | |
app.logger.debug(f"{request.remote_addr} \"{request.method} {request.url}\" {response.status_code}") |
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.
Yes.
Overview
Fixes
Testing
Test Results
Note: in test results, the exception handling messages are different based on how the report was authored. It is out of scope of the WMA to adjust report logic.