-
Notifications
You must be signed in to change notification settings - Fork 541
feat: add cloudwatch logs URL to lambda additional data #618
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
feat: add cloudwatch logs URL to lambda additional data #618
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! two smaller changes though
""" | ||
start_time_str = (start_time - timedelta(seconds=1)).isoformat().split(".")[0] | ||
end_time_str = (datetime.now() + timedelta(seconds=2)).isoformat().split(".")[0] | ||
url = ''.join([ |
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.
Instead of this please do
url = (
"string 1"
"string 2"
)
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.
That syntax will only work with quoted strings, half of these are variables/functions. I can do a multiline string with .format
at the end, is that what you want?
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.
(fixed)
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.
yeah sorry I didn't even think about that. .format
is good
Returns: | ||
str -- AWS Console URL to logs. | ||
""" | ||
start_time_str = (start_time - timedelta(seconds=1)).isoformat().split(".")[0] |
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.
would prefer a strftime with explicit args here
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.
Ok, why? Happy to do it, just curious. could also do this:
.replace(microsecond=0).isoformat()
if you object to the .split('.')[0]
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.
just personal preference
oh yeah please also add tests for this and ensure linting passes |
4ace295
to
a1f2ed2
Compare
Awesome, thanks! |
Oh, uh... sorry, wasn't done. I just pushed up the tests. |
Damn, sorry. Could you PR those separately? |
As discussed on discord, this adds an additional "extra" for cloudwatch logs URL, log group, and log stream.
Not sure what if any tests are required for this change. Thoughts?