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

Fixes: #4560 ignore WSL Integration Error #5642

Merged
merged 5 commits into from
Oct 5, 2023

Conversation

dpmerrill
Copy link
Contributor

Display WSL Integration status in GUI, but ignore any failures at startup.

Signed-off-by: dmerrill <douglas.merrill@suse.com>
Signed-off-by: dmerrill <douglas.merrill@suse.com>
@gaktive gaktive requested a review from mook-as October 4, 2023 18:12
Comment on lines 253 to 255
return this.syncDistroSocketProxy(distro.name, !reason);
} catch (error) {
console.error(`syncSocketProxy ${ distro.name } Error: ${ error }`);
Copy link
Contributor

Choose a reason for hiding this comment

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

syncDistroSocketProxy is async, but this anonymous function is not; so the catch will not catch rejected promises, but the outer await Promise.all will (and still throw an exception).

Given the other changes, do you want to wrap syncDistroSocketProxy in try/catch instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice catch! Change has been checked in.

Signed-off-by: dmerrill <douglas.merrill@suse.com>
@dpmerrill dpmerrill requested a review from mook-as October 5, 2023 16:23
Copy link
Contributor

@mook-as mook-as left a comment

Choose a reason for hiding this comment

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

Just comment quibbling.

* distribution is started or stopped, as desired.
* @param distro The distribution to manage.
* @param shouldRun Whether the docker socket proxy should be running.
* @note this function can not throw because the callers are expecting promises.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* @note this function can not throw because the callers are expecting promises.
* @note this function must not throw.

The fact that the callers want promises isn't quite the issue; the fact that throwing breaks startup is…
(Using "must not" to avoid ambiguity, see also RFC 2119 though we're not following that here.)

Copy link
Contributor Author

@dpmerrill dpmerrill Oct 5, 2023

Choose a reason for hiding this comment

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

Makes sense. Change made.

* syncDistroDockerPlugin ensures that a plugin is accessible in the given distro.
* @param distro The distribution to manage.
* @param pluginName The plugin to validate.
* @note this function can not throw because the callers are expecting promises.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* @note this function can not throw because the callers are expecting promises.
* @note this function must not throw.

See above for explanation.

Copy link
Contributor Author

@dpmerrill dpmerrill Oct 5, 2023

Choose a reason for hiding this comment

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

Makes sense. Change made.

Signed-off-by: dmerrill <douglas.merrill@suse.com>
@dpmerrill dpmerrill requested a review from mook-as October 5, 2023 20:47
mook-as
mook-as previously approved these changes Oct 5, 2023
Copy link
Contributor

@mook-as mook-as left a comment

Choose a reason for hiding this comment

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

LGTM.

Sorry, we turned on warnings-as-errors for eslint, so now there's a lint error that you didn't introduce…

@jandubois
Copy link
Member

Sorry, we turned on warnings-as-errors for eslint, so now there's a lint error that you didn't introduce…

Please fix it anyways in your PR, to get the tests to pass! As Larry Wall once wrote:

The Golden Gate wasn't our fault either, but we still put a bridge across it.

Signed-off-by: dmerrill <douglas.merrill@suse.com>
@dpmerrill dpmerrill merged commit 99f6a78 into rancher-sandbox:main Oct 5, 2023
10 checks passed
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.

Rancher Desktop won't start and always chooses the Debian WSL distro for some reason
3 participants