-
Notifications
You must be signed in to change notification settings - Fork 304
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
Use symlink to shorten Lima config path #1458
Conversation
Fixes rancher-sandbox#797 Signed-off-by: Josh Soref <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This wouldn't actually solve the issue, as we're running limactl start
with the long path (and that's the one emitting the failure).
We will need to fix the nerdctl stub too, of course (because that also spawns limactl), but this isn't the full thing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This kind of setup should be done as soon as the directory is either created or known to exist. The problem with this solution is that we're going to run the same code every time someone runs nerdctl
.
So setting up the symlink after LAS/rd/lima
is first created should happen at
rancher-desktop/src/k8s-engine/lima.ts
Line 582 in e17f930
await fs.promises.mkdir(path.dirname(this.CONFIG_PATH), { recursive: true }); |
Creating a symlink when the LAS/rd/lima
already exists could actually be done on upgrades only by leveraging the settings update table at
rancher-desktop/src/config/settings.ts
Line 21 in e17f930
const CURRENT_SETTINGS_VERSION = 4; |
4:
at rancher-desktop/src/config/settings.ts
Line 255 in e17f930
}; |
else
part of the above code.
Or did I miss something?
There are a number of issues to think through:
I'm tempted to move this discussion into a new issue, as this should be hashed out before coding starts... 😄 |
Please move discussion to #1465 |
Closing this PR — this needs to be done in the TypeScript code (and will probably more discussion before we can agree on the details of the implementation). Thanks for getting this rolling! |
Fixes #797