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

nit: fix autostart not working #2942

Merged
merged 3 commits into from Jun 26, 2023
Merged

nit: fix autostart not working #2942

merged 3 commits into from Jun 26, 2023

Conversation

cdrage
Copy link
Contributor

@cdrage cdrage commented Jun 20, 2023

nit: fix autostart not working

What does this PR do?

Fixes autostart not working correctly.

This bug occurs during onMount when ProviderConfigured.svelte is loaded
and does runProvider without checking the autostart configuration.

Very small change / nit.

Screenshot/screencast of this PR

Screen.Recording.2023-06-20.at.1.50.49.PM.mov

What issues does this PR fix or reference?

Fixes #2781

How to test this PR?

  1. Disable autostart
  2. Stop podman machine
  3. Exit PD
  4. Start PD, it should still stay stopped.

Signed-off-by: Charlie Drage charlie@charliedrage.com

@cdrage cdrage requested review from a team and benoitf as code owners June 20, 2023 17:41
@cdrage cdrage requested review from jeffmaury and lstocchi and removed request for a team June 20, 2023 17:41
@cdrage
Copy link
Contributor Author

cdrage commented Jun 20, 2023

Tests failing, investigating fixed

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

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.

There is a empty kind file which should be removed.

@cdrage cdrage force-pushed the autostart-bug branch 2 times, most recently from d42cf18 to f24cd74 Compare June 22, 2023 12:46
@deboer-tim
Copy link
Collaborator

Something has been nagging me, but please let me know if I'm misunderstanding. My main concern: why is there anything in onMount() in the first place?

If I have auto-start off, then shouldn't the provider only start based on a user action? (if the user action makes this page appear, also odd that onMount() would be used)
If I have auto-start on, then don't I want the provider to start even if I don't go to the dashboard? e.g. it should start if only the tray is open, or we have some future path like the protocol handler where the dashboard is not the first page I see.

It seems like the auto-start should be in a completely different place in either case (not tied to a particular UI page) and this onMount() doesn't exist.

@cdrage
Copy link
Contributor Author

cdrage commented Jun 22, 2023

Something has been nagging me, but please let me know if I'm misunderstanding. My main concern: why is there anything in onMount() in the first place?

If I have auto-start off, then shouldn't the provider only start based on a user action? (if the user action makes this page appear, also odd that onMount() would be used) If I have auto-start on, then don't I want the provider to start even if I don't go to the dashboard? e.g. it should start if only the tray is open, or we have some future path like the protocol handler where the dashboard is not the first page I see.

It seems like the auto-start should be in a completely different place in either case (not tied to a particular UI page) and this onMount() doesn't exist.

100% agree and it's odd that it is there in the first place.. @lstocchi and I are having this discussion now on slack

@cdrage cdrage marked this pull request as draft June 22, 2023 14:28
@cdrage
Copy link
Contributor Author

cdrage commented Jun 22, 2023

Something has been nagging me, but please let me know if I'm misunderstanding. My main concern: why is there anything in onMount() in the first place?

If I have auto-start off, then shouldn't the provider only start based on a user action? (if the user action makes this page appear, also odd that onMount() would be used) If I have auto-start on, then don't I want the provider to start even if I don't go to the dashboard? e.g. it should start if only the tray is open, or we have some future path like the protocol handler where the dashboard is not the first page I see.

It seems like the auto-start should be in a completely different place in either case (not tied to a particular UI page) and this onMount() doesn't exist.

So the TLDR on slack is that we require onMount() to exist in order to be able to initialize / interact with the user in terms of initializing each provider. For example, we have initialize and start for Podman / button interaction before we use startProvider().

Makes sense as future features we'd be able to ask the user / interact before actually launching.

I've updated the PR to draft and I'll push some new code.

@cdrage cdrage marked this pull request as ready for review June 22, 2023 15:31
@cdrage
Copy link
Contributor Author

cdrage commented Jun 22, 2023

Ready for review!

"Run Podman" or "Run Provider" button's will appear when autostart is false and Podman Desktop appears for the first time.

All it needed was an update to runAtStart = autostart && initializationContext.mode === InitializeAndStartMode;

Ready for review @lstocchi

@lstocchi
Copy link
Contributor

lstocchi commented Jun 23, 2023

Ready for review!

"Run Podman" or "Run Provider" button's will appear when autostart is false and Podman Desktop appears for the first time.

All it needed was an update to runAtStart = autostart && initializationContext.mode === InitializeAndStartMode;

I still face the same issue. If i click on initialize and start i expect that it creates a machine and start it. But if the autostart flag is set to false, the machine is only created and not started.

init-start-fail

the question is how it should work. Bc if the autostart flag has always priority then it works fine.

@benoitf
Copy link
Collaborator

benoitf commented Jun 23, 2023

I gave a quick look

@lstocchi My question is that it looks like the Dashboard page initialize a map called providerInitContexts setting the mode to { mode: InitializeAndStartMode }; without any user action so ProviderConfigured will always see mode is InitializeAndStartMode and will run the machine

I'm wondering why we initialize data to InitializeAndStartMode I suppose the value should be set only if user click on 'Init' or 'Init and Start' (this is what is done but it is also the default value??) and then we should initialize the context to something 'empty/none/do nothing'

like in Dashboard.svelte:

    context = { mode: DoNothingMode };

and ProviderInitUtils having

export const DoNothingMode = 'DoNothing';
export const InitializeOnlyMode = 'Initialize';
export const InitializeAndStartMode = 'Initialize and start';
export type InitializationMode = typeof NoneMode |typeof InitializeOnlyMode | typeof InitializeAndStartMode;

@cdrage
Copy link
Contributor Author

cdrage commented Jun 23, 2023

Ready for review!
"Run Podman" or "Run Provider" button's will appear when autostart is false and Podman Desktop appears for the first time.
All it needed was an update to runAtStart = autostart && initializationContext.mode === InitializeAndStartMode;

I still face the same issue. If i click on initialize and start i expect that it creates a machine and start it. But if the autostart flag is set to false, the machine is only created and not started.

init-start-fail

the question is how it should work. Bc if the autostart flag has always priority then it works fine.

We have no way of knowing if it's been started for the first time, so I think we'd have to update the dashboard with a new value similar to Initialize and Start, what do you think?

EDIT: I see @benoitf and I commented at the same time. I'll update this PR so that we add a new value of "DoNothing" and only apply Init / Init and start when the user actually clicks the value.

@lstocchi
Copy link
Contributor

I gave a quick look

@lstocchi My question is that it looks like the Dashboard page initialize a map called providerInitContexts setting the mode to { mode: InitializeAndStartMode }; without any user action so ProviderConfigured will always see mode is InitializeAndStartMode and will run the machine

I'm wondering why we initialize data to InitializeAndStartMode I suppose the value should be set only if user click on 'Init' or 'Init and Start' (this is what is done but it is also the default value??) and then we should initialize the context to something 'empty/none/do nothing'

like in Dashboard.svelte:

    context = { mode: DoNothingMode };

and ProviderInitUtils having

export const DoNothingMode = 'DoNothing';
export const InitializeOnlyMode = 'Initialize';
export const InitializeAndStartMode = 'Initialize and start';
export type InitializationMode = typeof NoneMode |typeof InitializeOnlyMode | typeof InitializeAndStartMode;

Correct. At that time i initialize it to InitializeAndStartMode as i didn't think it could cause any problem. But by changing the default value it should solve the issue

### What does this PR do?

Fixes autostart not working correctly.

This bug occurs during onMount when ProviderConfigured.svelte is loaded
and does runProvider without checking the autostart configuration.

Very small change / nit.

### Screenshot/screencast of this PR

<!-- Please include a screenshot or a screencast explaining what is doing this PR -->

### What issues does this PR fix or reference?

<!-- Please include any related issue from Podman Desktop repository (or from another issue tracker).
-->

Fixes containers#2781

### How to test this PR?

1. Disable autostart
2. Stop podman machine
3. Exit PD
4. Start PD, it should still stay stopped.

<!-- Please explain steps to reproduce -->

Signed-off-by: Charlie Drage <charlie@charliedrage.com>
Signed-off-by: Charlie Drage <charlie@charliedrage.com>
@cdrage
Copy link
Contributor Author

cdrage commented Jun 23, 2023

Added DoNothing + changed the line to: runAtStart = autostart || initializationContext.mode === InitializeAndStartMode;

Tests done:

  1. Disable autostart: When pressing initialize + start, podman starts correctly
  2. Stop podman + restart podman desktop. Podman is still stopped.
  3. Enable autostart, start podman desktop, Podman machine starts correctly

Signed-off-by: Charlie Drage <charlie@charliedrage.com>
@cdrage
Copy link
Contributor Author

cdrage commented Jun 26, 2023

Removed the lingering comment and have marked this as auto-merge

@cdrage cdrage merged commit 9c536df into containers:main Jun 26, 2023
8 checks passed
@podman-desktop-bot podman-desktop-bot added this to the 1.2.0 milestone Jun 26, 2023
mairin pushed a commit to mairin/podman-desktop that referenced this pull request Jul 7, 2023
* nit: fix autostart not working

### What does this PR do?

Fixes autostart not working correctly.

This bug occurs during onMount when ProviderConfigured.svelte is loaded
and does runProvider without checking the autostart configuration.

Very small change / nit.

### Screenshot/screencast of this PR

<!-- Please include a screenshot or a screencast explaining what is doing this PR -->

### What issues does this PR fix or reference?

<!-- Please include any related issue from Podman Desktop repository (or from another issue tracker).
-->

Fixes containers#2781

### How to test this PR?

1. Disable autostart
2. Stop podman machine
3. Exit PD
4. Start PD, it should still stay stopped.

<!-- Please explain steps to reproduce -->

Signed-off-by: Charlie Drage <charlie@charliedrage.com>

* add donothing

Signed-off-by: Charlie Drage <charlie@charliedrage.com>

* update

Signed-off-by: Charlie Drage <charlie@charliedrage.com>

---------

Signed-off-by: Charlie Drage <charlie@charliedrage.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.

Podman Machine AutoStarts EVEN when AutoStart button is set to Disabled
6 participants