-
Notifications
You must be signed in to change notification settings - Fork 55
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: allow reading logs from non-project paths #444
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.
Left some minor comments
@@ -46,11 +46,12 @@ | |||
) | |||
|
|||
|
|||
def logger_name_from_path(path): | |||
def logger_name_from_path(path, project=None): |
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.
nit: Perhaps I am not well familiar with Python, but is there a way to create another logger_name_from_path
function with project
parameter? I think that it would be better then having parameters with default values
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.
I think this is the Pythonic way to do it. Especially since this function is just directly calling another function. If it wasn't a public function already in use, I would prefer to remove this one altogether
What's the argument against default parameters?
logger_name = logger_name_from_path(logger_fullname, client.project) | ||
logger = loggers[logger_fullname] = client.logger(logger_name) | ||
except ValueError: | ||
# log name is not scoped to a project. Leave logger as None |
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.
Is this comment true? Seems that logger is initialized with value returned from loggers.get(logger_fullname)
call...
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.
loggers.get(logger_fullname)
will return None
if the loggers dict doesn't contain logger_fullname
If it's None
. Lines 171 to 173 will try to contruct a new logger associated with logger_name
, which will work if logger_name
is a project id. If it's an organization or a folder, it will throw an exception. In that case, we can just leave the logger as empty and move on
Previously, the entry-parsing code assumed all logs originated from a project, but the list_logs API allows reading from folders, organizations, and billingIds as well. If a user attempted to read from one of these non-project paths, the library would fail to build the LogEntry object and crash. This PR fixes that issue, and is more forgiving in its entry parsing logic.
Now, if a log is found with a parent path that does not represent the active project, the
entry.logger
field will beNone
.Fixes #399