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.

Sign up for GitHub

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

Jump to bottom

include stderr and stdout in CommandExecutionException #816

Merged
merged 12 commits into from
Mar 23, 2020
Merged

include stderr and stdout in CommandExecutionException #816

merged 12 commits into from
Mar 23, 2020

Conversation

elharo
Copy link
Contributor

@elharo elharo commented Nov 8, 2019 •
edited
Loading

saturnism
saturnism reviewed Nov 8, 2019
View reviewed changes
src/main/java/com/google/cloud/tools/managedcloudsdk/command/CommandCaller.java
} catch (InterruptedException | ExecutionException ignored) {
stdErr = "stderr collection interrupted";
}
throw new CommandExecutionException(stdOut + "\n" + stdErr, ex);
Copy link

Choose a reason for hiding this comment

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

in CommandExitException, error is stored in a separate field instead of the actual message field of the exception. I feel that's cleaner. I'd also suggest that stdout and stderr be kept in 2 different fields and let the caller render it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@codecov
Copy link

codecov bot commented Nov 11, 2019 •
edited
Loading

Codecov Report

Merging #816 into master will decrease coverage by <.01%.
The diff coverage is 69.23%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #816      +/-   ##
============================================
- Coverage     77.13%   77.12%   -0.01%     
- Complexity      617      618       +1     
============================================
  Files            98       98              
  Lines          2449     2457       +8     
  Branches        325      325              
============================================
+ Hits           1889     1895       +6     
- Misses          412      414       +2     
  Partials        148      148
Impacted Files Coverage Δ Complexity Δ
...gedcloudsdk/command/CommandExecutionException.java 50% <100%> (+50%) 1 <1> (+1) ⬆️
...tools/managedcloudsdk/process/ProcessExecutor.java 96% <100%> (-0.3%) 8 <0> (ø)
...d/tools/managedcloudsdk/command/CommandCaller.java 74.19% <55.55%> (+0.28%) 4 <0> (ø) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1edd286...04c4ebc. Read the comment docs.

elharo
elharo commented Nov 15, 2019
View reviewed changes
Copy link
Contributor Author

@elharo elharo left a comment

Choose a reason for hiding this comment

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

Is this worth proceeding with or should we close it as WAI?

@loosebazooka
Copy link
Contributor

I think it's fine... will review when I get a chance. @saturnism @chanseokoh any comments?

@chanseokoh
Copy link
Contributor

Nothing much from my side besides this minor Javadoc comment: #816 (comment)

@codecov-io
Copy link

codecov-io commented Dec 21, 2019 •
edited by codecov bot
Loading

Codecov Report

Merging #816 into master will increase coverage by 0.11%.
The diff coverage is 80.95%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #816      +/-   ##
============================================
+ Coverage     78.35%   78.47%   +0.11%     
- Complexity      608      612       +4     
============================================
  Files            97       97              
  Lines          2398     2411      +13     
  Branches        282      282              
============================================
+ Hits           1879     1892      +13     
  Misses          411      411              
  Partials        108      108
Impacted Files Coverage Δ Complexity Δ
...gedcloudsdk/command/CommandExecutionException.java 100% <100%> (+100%) 4 <3> (+4) ⬆️
.../managedcloudsdk/command/CommandExitException.java 100% <100%> (ø) 4 <1> (ø) ⬇️
...tools/managedcloudsdk/process/ProcessExecutor.java 96% <100%> (-0.3%) 8 <0> (ø)
...d/tools/managedcloudsdk/command/CommandCaller.java 74.19% <55.55%> (+0.28%) 4 <0> (ø) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 93de30d...a7b2154. Read the comment docs.

elharo
elharo commented Mar 14, 2020
View reviewed changes
Copy link
Contributor Author

@elharo elharo left a comment

Choose a reason for hiding this comment

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

Ping

@elharo elharo merged commit d9454c3 into master Mar 23, 2020
@elharo elharo deleted the i814 branch March 23, 2020 20:13
JoeWang1127 pushed a commit that referenced this pull request Nov 29, 2023
@elharo
include stderr and stdout in CommandExecutionException (#816)
85df129 
* include stderr and stdout in CommandExecutionException
* update JavaDoc
* add getErrorLog method
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@saturnism saturnism saturnism left review comments

@chanseokoh chanseokoh chanseokoh left review comments

@loosebazooka loosebazooka loosebazooka approved these changes

Assignees
No one assigned
Labels
cla: yes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@elharo @loosebazooka @chanseokoh @codecov-io @saturnism @googlebot