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

fix: keep stdout and stderr in the error object for Docker Desktop extensions #3014

Merged
merged 3 commits into from Jul 6, 2023

Conversation

benoitf
Copy link
Collaborator

@benoitf benoitf commented Jun 26, 2023

What does this PR do?

Keep stdout ad stderr in the error object for Docker Desktop extensions
it looks like some extensions expect to find it

Screenshot/screencast of this PR

N/A

What issues does this PR fix or reference?

Fixes #3017

How to test this PR?

@benoitf benoitf requested a review from a team as a code owner June 26, 2023 19:34
@benoitf benoitf requested review from jeffmaury and cdrage and removed request for a team June 26, 2023 19:34
Copy link
Contributor

@jeffmaury jeffmaury left a comment

Choose a reason for hiding this comment

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

The code looks weird to me because in case of error this first method resolves an error and not an exec result and the caller will check for result.error which I don't understand where it is set

@benoitf
Copy link
Collaborator Author

benoitf commented Jun 28, 2023

@jeffmaury should be fixed now

Copy link
Contributor

@jeffmaury jeffmaury left a comment

Choose a reason for hiding this comment

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

Sorry but still not getting it: if error is raised (bad command or process killed) then exit code will be 0 as error is resolved and close event won't be processed. Also I don't see when result.error is set

…ensions

it looks like some extensions expect to find it

Signed-off-by: Florent Benoit <fbenoit@redhat.com>
…top extensions

Signed-off-by: Florent Benoit <fbenoit@redhat.com>
…top extensions

Signed-off-by: Florent Benoit <fbenoit@redhat.com>
@benoitf
Copy link
Collaborator Author

benoitf commented Jul 5, 2023

@jeffmaury looks like there was dead code. I removed the part about the .error field

@benoitf benoitf changed the title fix: keep stdout ad stderr in the error object for Docker Desktop extensions fix: keep stdout and stderr in the error object for Docker Desktop extensions Jul 5, 2023
Copy link
Contributor

@lstocchi lstocchi left a comment

Choose a reason for hiding this comment

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

LGTM

@benoitf benoitf merged commit b5650cc into containers:main Jul 6, 2023
8 checks passed
@podman-desktop-bot podman-desktop-bot added this to the 1.2.0 milestone Jul 6, 2023
mairin pushed a commit to mairin/podman-desktop that referenced this pull request Jul 7, 2023
…tensions (containers#3014)

* fix: keep stdout ad stderr in the error object for Docker Desktop extensions

Signed-off-by: Florent Benoit <fbenoit@redhat.com>
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.

microcks docker desktop extension is not working in Podman Desktop
5 participants