Skip to content
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

Support V3 log interface #284

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

colemanja91
Copy link

Add support for Serverless V3 logging (per #279 )

Copy link
Contributor

@pgrzesik pgrzesik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot @colemanja91 🙇 It looks good so far - do you plan any additional changes to it?

@colemanja91 colemanja91 marked this pull request as ready for review January 17, 2022 20:22
@colemanja91
Copy link
Author

@pgrzesik I kept it in WIP while doing some local testing with v3 - seems ready for a review!

@colemanja91 colemanja91 changed the title WIP: support V3 log interface Support V3 log interface Jan 18, 2022
@pgrzesik
Copy link
Contributor

Thanks @colemanja91 👍 I'll run it locally to see if there's anything missing in the workflow. 👍

@pgrzesik
Copy link
Contributor

Hello @colemanja91 - how did you verify it's properly working on local? I'm working with your branch locally and it doesn't seem to take the effect. Looking at the code it seems like the original setup of v3Utils log-related functions is happening at a wrong place or it's misused. You're using references to these helper functions on provider, but they're never assigned to provider. Am I missing something?

Copy link
Contributor

@pgrzesik pgrzesik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @colemanja91, looks good in general, but I'm afraid the setup is a bit wrong - please let me know what do you think

this.serverless = serverless;
this.options = options;

if (v3Utils) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is setup on GoogleIndex level where everywhere in the code you reference it from this.provider - I believe it's not reachable on GoogleProvider level.

@@ -22,7 +22,11 @@ module.exports = {

let vpcEgress = funcObject.vpcEgress || this.serverless.service.provider.vpcEgress;

this.serverless.cli.log(`Compiling function "${functionName}"...`);
if (this.log) {
this.log.notice(`Compiling function "${functionName}"...`);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like a candidate for progress as it can take some time

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants