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

Log error (stdout and stderr) on command failures (non-zero exits) for Managed Cloud SDK processes #991

Open
saturnism opened this issue Nov 8, 2019 · 8 comments

Comments

@saturnism
Copy link

Currently, when command fails (such as ManagedCloudSDK.isUpToDate()) fails, there is no insight into detailed error message, even though, error message is captured in CommandExitException.

https://github.com/GoogleCloudPlatform/appengine-plugins-core/blob/master/src/main/java/com/google/cloud/tools/managedcloudsdk/ManagedCloudSdk.java#L162

Suggest that upon CommandExitException, log error with high priority and/or propagate to the wrapping Exception, or, use it as part of the message of CommandExitException

@saturnism
Copy link
Author

saturnism commented Nov 8, 2019

@elharo
Copy link
Contributor

elharo commented Nov 8, 2019

This makes sense though I don't think it's diagnosed quite right. The problem is that CommandCaller doesn't save stderr into the exception. It should.

Logging here we don't need to do. Including more information in the exception is the right approach.

@elharo elharo self-assigned this Nov 8, 2019
@saturnism
Copy link
Author

saturnism commented Nov 8, 2019

@elharo it does save error log into CommandExitException. I can see the error message. but no where in the stack trace and/or execution actually displayed it.

@saturnism
Copy link
Author

Although, the error should probably to be logged and/or surfaced by the consumer of this API instead.

@elharo
Copy link
Contributor

elharo commented Nov 8, 2019

That's a decision for the dependent of this library to make. This library does not attempt to talk to end users.

@elharo
Copy link
Contributor

elharo commented Nov 11, 2019

As described here, I'm no longer sure there is an issue for appengine-plugins-core to resolve. This library makes that information available downstream. It's up to the individual callers to make use of it. Perhaps this bug should be addressed in app-maven-pluign and cloud-code-intellij?

@chanseokoh chanseokoh transferred this issue from GoogleCloudPlatform/appengine-plugins Nov 11, 2019
@chanseokoh
Copy link
Contributor

Issue transferred from appengine-plugins-core to app-maven-plugin. I'll create a new one for app-gradle-plugin.

@chanseokoh chanseokoh changed the title Log error on command failures (non-zero exits) Log error (stdout and stderr) on command failures (non-zero exits) for Managed Cloud SDK processes Nov 11, 2019
@elharo elharo removed their assignment Jul 27, 2021
@elefeint
Copy link
Contributor

Cross-commenting from the gradle issue; not planning to close the individual plugin ones.

I think this should be resolved here in appengine-plugins-core.

Yes, ManagedSdkVerificationException contains the cause and can return it if prompted, but then it's on the caller (maven and gradle plugins) to get the cause exception, compare its type to CommandExecutionException and get the output.

It would be easier if CommandExecutionException.getErrorLog() got logged before it's wrapped in ManagedSdkVerificationException.

@JoeWang1127 JoeWang1127 transferred this issue from GoogleCloudPlatform/app-maven-plugin Jan 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants