Log errors to the global error log when they occur during building. #89

Merged
merged 3 commits into from Apr 17, 2012

Conversation

Projects
None yet
2 participants
@davidfowl
Member

davidfowl commented Apr 17, 2012

  • Don't duplicate output for msbuild and npm install.
  • Added assertions to function tests.
Log errors to the global error log when they occur during building.
- Don't duplicate output for msbuild and npm install.
- Added assertions to function tests.

@ghost ghost assigned davidebbo Apr 17, 2012

@davidebbo

This comment has been minimized.

Show comment
Hide comment
@davidebbo

davidebbo Apr 17, 2012

Member

I never noticed output was duplicated.

Member

davidebbo commented Apr 17, 2012

I never noticed output was duplicated.

@davidfowl

This comment has been minimized.

Show comment
Hide comment
@davidfowl

davidfowl Apr 17, 2012

Member

It wasn't, and this change doesn't introduce that (which it would if I were to blindly log all errors).

Member

davidfowl commented Apr 17, 2012

It wasn't, and this change doesn't introduce that (which it would if I were to blindly log all errors).

Kudu.Core/Deployment/BasicBuilder.cs
+ // The reason we don't log the real exception is because the 'live output' when downloding
+ // npm packages has already been captured.
+ context.GlobalLogger.Log(String.Empty, LogEntryType.Error);
+

This comment has been minimized.

@davidebbo

davidebbo Apr 17, 2012

Member

But didn't we run into cases where npm was dying without outputting anything? Or is that a separate issue not yet addressed?

@davidebbo

davidebbo Apr 17, 2012

Member

But didn't we run into cases where npm was dying without outputting anything? Or is that a separate issue not yet addressed?

This comment has been minimized.

@davidebbo

davidebbo Apr 17, 2012

Member

Also, what's not clear from the comment is what exactly does this add to the log. Obviously, you wouldn't do it if it had no effect :)

@davidebbo

davidebbo Apr 17, 2012

Member

Also, what's not clear from the comment is what exactly does this add to the log. Obviously, you wouldn't do it if it had no effect :)

This comment has been minimized.

@davidfowl

davidfowl Apr 17, 2012

Member

The npm issue is addressed. The output you saw was the error stream which was correct. This change makes the "Error occured" show up in the console.

It's a little hacky (hence the HACK:) since we write live output directly to the console and the GlobalLogger is also attached to console. So I need to Log an error here so that the code in kudu.exe knows that there was an overall error (Since this exception doesn't make it's way up to the exe),

@davidfowl

davidfowl Apr 17, 2012

Member

The npm issue is addressed. The output you saw was the error stream which was correct. This change makes the "Error occured" show up in the console.

It's a little hacky (hence the HACK:) since we write live output directly to the console and the GlobalLogger is also attached to console. So I need to Log an error here so that the code in kudu.exe knows that there was an overall error (Since this exception doesn't make it's way up to the exe),

This comment has been minimized.

@davidfowl

davidfowl Apr 17, 2012

Member

This is a hacky way of doing something like MarkDeploymentAsFailed().

@davidfowl

davidfowl Apr 17, 2012

Member

This is a hacky way of doing something like MarkDeploymentAsFailed().

This comment has been minimized.

@davidebbo

davidebbo Apr 17, 2012

Member

Ah I see.

@davidebbo

davidebbo Apr 17, 2012

Member

Ah I see.

@@ -8,6 +8,7 @@ public class DeploymentContext
public IDeploymentManifestWriter ManifestWriter { get; set; }
public ITracer Tracer { get; set; }
public ILogger Logger { get; set; }
+ public ILogger GlobalLogger { get; set; }

This comment has been minimized.

@davidebbo

davidebbo Apr 17, 2012

Member

Basic comments about the semantic of these 3 things would be nice. It's pretty confusing when you don't know the code well.

@davidebbo

davidebbo Apr 17, 2012

Member

Basic comments about the semantic of these 3 things would be nice. It's pretty confusing when you don't know the code well.

This comment has been minimized.

@davidfowl

davidfowl Apr 17, 2012

Member

Done.

@davidfowl davidfowl merged commit 58bc7b4 into master Apr 17, 2012

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment