-
Notifications
You must be signed in to change notification settings - Fork 12
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
refactor: create getMinikubeAdditionalEnvs method for minikube exec #224
refactor: create getMinikubeAdditionalEnvs method for minikube exec #224
Conversation
Signed-off-by: axel7083 <[email protected]>
my point was that it's not only a type checking issue. It's still changing how the variables are given as we remove content (even it is might be added by a 3rd party later) fixing only typechecking is done by changing env to Here it's more related to avoiding to pass original env values as they're added by Podman Desktop core. |
src/create-cluster.ts
Outdated
env.MINIKUBE_HOME = getMinikubeHome(); | ||
const env: Record<string, string> = { | ||
PATH: getMinikubePath(), | ||
MINIKUBE_HOME: getMinikubeHome() ?? '', |
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.
should it be
- if it exists we add it to env object else we don't
rather than
-adding an empty string value
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.
Yes, I agree, fixed 👍
src/extension.ts
Outdated
await extensionApi.process.exec(minikubeCli, ['start', '--profile', cluster.name], { env }); | ||
await extensionApi.process.exec(minikubeCli, ['start', '--profile', cluster.name], { | ||
env: { | ||
PATH: getMinikubePath(), |
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.
not related to this PR but it's strange sometimes we have both MINIKUBE_HOME and sometimes only the PATH
also should we call a custom utility method that will insert minikubePath and MinikubeHome instead of doing that at several places
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.
I think this is the core issue actually, thanks for pointing that out, the code should be centralized, and we should have a single function that create the envs required
Signed-off-by: axel7083 <[email protected]>
Signed-off-by: axel7083 <[email protected]>
Split up of #223.
Related issues
Part of #154
process.env
type issueThis PR remove the
process.env
from the envs passed to the exec api because its type is incompatible with the API, and is unessessary.When running
tsc
we see the following errorIncompatible type
process.env is incompatible with our RunOptions#env.
process.env
is typed asProcessEnv
, which is the followingThis is not compatible with our
RunOptions#env
defined asUnnecessary
If only the type was incompatible I would have forced the typecheck by casting it, or using some Object.assign magic, but this is not necessary as podman desktop core is already doing it for us.
Podman desktop automatically adds it see main/src/plugin/util/exec.ts#L50