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

Resolves #33 Add MINIKUBE_HOME configuration #48

Merged
merged 1 commit into from
Jul 25, 2024
Merged

Resolves #33 Add MINIKUBE_HOME configuration #48

merged 1 commit into from
Jul 25, 2024

Conversation

tony-sol
Copy link
Contributor

This is second try of making extension follows actual MINIKUBE_HOME instead of default.

Previous context

Extension preferences

@tony-sol tony-sol requested review from benoitf and a team as code owners June 11, 2024 18:22
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.

Overall LGTM.
But why did you change the name of the creation fields? We are inside the create minikube page, it is obvious that the fields are related to it.
I would change them back or, if others agree with you, open a new PR to handle them.
image

@tony-sol
Copy link
Contributor Author

But why did you change the name of the creation fields?

Oh, sorry, didn't notice that it was in commit. Revert this unintended changes.

@lstocchi
Copy link
Contributor

But why did you change the name of the creation fields?

Oh, sorry, didn't notice that it was in commit. Revert this unintended changes.

Thank you @tony-sol . It looks like you also pushed your yarn.lock but as you didn't add any deps this should not change. Can you revert it? Thank

@tony-sol
Copy link
Contributor Author

But why did you change the name of the creation fields?

Oh, sorry, didn't notice that it was in commit. Revert this unintended changes.

Thank you @tony-sol . It looks like you also pushed your yarn.lock but as you didn't add any deps this should not change. Can you revert it? Thank

Done ^_^

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

@tony-sol
Copy link
Contributor Author

just syncing with upstream

@tony-sol
Copy link
Contributor Author

@benoitf may i ask you for review pls?

Comment on lines +28 to +37
const { config } = vi.hoisted(() => {
return {
config: {
get: vi.fn(),
has: vi.fn(),
update: vi.fn(),
},
};
});

Copy link
Contributor

Choose a reason for hiding this comment

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

we don't need the hoisted block, you can return mock (vi.fn) directly in getConfiguration method

using the vi.mockImplementation we have typechecking while here we don't

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've tried, but not figured out, how to make this return different value on different tests. I tried to do like other tests in this project and podman-desktop project, but without success 😞

a little help, like example would be awesome

src/util.ts Outdated
Comment on lines 52 to 62
export function getMinikubeHome(): string {
const minikubeHome = minikubeConfiguration.get<string>('home');
// Check env if configuration is not applied in UI
if (!minikubeHome) {
const env = process.env;
return env.MINIKUBE_HOME;
} else {
return minikubeHome;
}
}

Copy link
Contributor

Choose a reason for hiding this comment

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

the function in case of missing env var will return undefined so the signature seems invalid (should be string | undefined)

@odockal
Copy link

odockal commented Jul 4, 2024

@tony-sol Can you please rebase? Seems like we got conflicts there and CI is not passing...

@tony-sol
Copy link
Contributor Author

tony-sol commented Jul 4, 2024

@tony-sol Can you please rebase? Seems like we got conflicts there and CI is not passing...

@odockal check pls

@odockal
Copy link

odockal commented Jul 10, 2024

@tony-sol

error Your lockfile needs to be updated, but yarn was run with `--frozen-lockfile`.
``` Seems like your yarn.lock is out of date.

@jeffmaury
Copy link
Contributor

@tony-sol can you remote yarn.lock from this PR as it should not be updated

Signed-off-by: Tony-Sol <tony.kent.nar.earth@gmail.com>
@tony-sol
Copy link
Contributor Author

@tony-sol can you remote yarn.lock from this PR as it should not be updated

done

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.

LGTM

@jeffmaury jeffmaury merged commit 3782297 into podman-desktop:main Jul 25, 2024
4 checks passed
@jeffmaury
Copy link
Contributor

Thanks for this, @tony-sol

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.

5 participants