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.

Sign up for GitHub

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

Jump to bottom

Support skipWaiting initially but keep periodically checking afterwards #6

Open
damdo opened this issue Jan 14, 2025 · 6 comments · May be fixed by #7
Open

Support skipWaiting initially but keep periodically checking afterwards #6

damdo opened this issue Jan 14, 2025 · 6 comments · May be fixed by #7

Comments

@damdo
Copy link
Collaborator

damdo commented Jan 14, 2025

@mpl : I find it a bit inconvenient that there is no mode where you are in the periodic case, but where there is also an immediate run nonetheless.
So I was thinking the code could become something like:

log.Print("skipping waiting, performing an immediate updateProcess")
if err := updateProcess(ctx, gusCli, machineID, o.gusServer, sbomHash, o.destinationDir, httpPassword, httpPort); err != nil {
// If the updateProcess fails we exit with an error
// so that gokrazy supervisor will restart the process.
return fmt.Errorf("error performing updateProcess: %v", err)
}

if o.skipWaiting {
// If the updateProcess doesn't error
// we happily return to terminate the process.
return nil
}

Currently the logic supports:
1 - initial waiting for 1 frequency period, then check for update (conditionally perform one), then wait again, and repeat infinitely
2 - skip initial waiting by specifying the skipWaiting, check for update (conditionally perform one), then exit(0)

We should modify mode 2 or support a third mode that skips the initial waiting, perform the check for an update, and then repeat this indefinitely.
@damdo
Copy link
Collaborator Author

damdo commented Jan 14, 2025

@mpl I agree with having this mode, and I've also changed this logic slightly locally when I switched modes because I also found out I wanted this instead of the current mode 2 behaviour, which is useful for e2e/ci testing but less in production.

So I'm happy to change the logic to achieve the use case of: skip initial waiting + keep periodically checking.

Although I would differ a bit from what you are suggesting.

Doing what you are suggesting changes the current meaning we put into skipWaiting.
The skipWaiting flag currently means: skip the waiting period for the first check
Rather than: do update once and then skip waiting = stop checking.

I would be more inclined to add a new say: runOnce flag, that does the exiting after the first updateProcess().
In this case we can do the skipWaiting independently from the runOnce and independently from the default mode.

WDYT?

@damdo damdo mentioned this issue Jan 14, 2025
Merged
@mpl
Copy link
Contributor

mpl commented Jan 14, 2025

@mpl I agree with having this mode, and I've also changed this logic slightly locally when I switched modes because I also found out I wanted this instead of the current mode 2 behaviour, which is useful for e2e/ci testing but less in production.

So I'm happy to change the logic to achieve the use case of: skip initial waiting + keep periodically checking.

Although I would differ a bit from what you are suggesting.

Doing what you are suggesting changes the current meaning we put into skipWaiting. The skipWaiting flag currently means: skip the waiting period for the first check Rather than: do update once and then skip waiting = stop checking.

I'm confused. Do you agree that at the moment the description of skipWaiting is a lie, because it does not specify that we exit after the first run (which is unexpected imho)?

I would be more inclined to add a new say: runOnce flag, that does the exiting after the first updateProcess(). In this case we can do the skipWaiting independently from the runOnce and independently from the default mode.

So just to be clear:

  • you would fix skipWaiting so that when enabled it does not force an exit right after the first run. All it would impact is whether we wait or not before a first run, correct?
  • and then you would add runOnce which controls whether we exit or not right after the first run?

If yes, then why not.
But tbh, (with the new behaviour we've just described) I wouldn't see much point in keeping the skipWaiting=false behaviour. Why would it ever a problem to always try an immediate update run?

@damdo
Copy link
Collaborator Author

damdo commented Jan 14, 2025

So just to be clear: you would fix skipWaiting so that when enabled it does not force an exit right after the first run. All it would impact is whether we wait or not before a first run, correct?
and then you would add runOnce which controls whether we exit or not right after the first run?

Yes, that was what I was thinking.

But tbh, (with the new behaviour we've just described) I wouldn't see much point in keeping the skipWaiting=false behaviour. Why would it ever a problem to always try an immediate update run?

Yes that could make sense too. I don't think there was any special reason why we did that initially. I'm happy with changing it.

@stapelberg
Copy link
Contributor

Personally, I would find it easiest to see a code change to discuss/review, so once you two reach agreement, feel free to loop me into a PR review :)

@mpl
Copy link
Contributor

mpl commented Jan 14, 2025

So just to be clear: you would fix skipWaiting so that when enabled it does not force an exit right after the first run. All it would impact is whether we wait or not before a first run, correct?
and then you would add runOnce which controls whether we exit or not right after the first run?

Yes, that was what I was thinking.

But tbh, (with the new behaviour we've just described) I wouldn't see much point in keeping the skipWaiting=false behaviour. Why would it ever a problem to always try an immediate update run?

Yes that could make sense too. I don't think there was any special reason why we did that initially. I'm happy with changing it.

Cool. Want me to open a PR?

@damdo
Copy link
Collaborator Author

damdo commented Jan 15, 2025

@mpl sounds good, thanks!

mpl added a commit to mpl/selfupdate that referenced this issue Jan 15, 2025
@mpl
New option: run_once, and removed skip_waiting
ffe4bc6 
- new default behaviour: an update check is always attempted right from
  the start, regardless of check_frequency value.
- run_once: controls whether we exit right after the initial update
  check.
- skip_waiting: removed

Fixes gokrazy#6
@mpl mpl linked a pull request Jan 15, 2025 that will close this issue
Open
mpl added a commit to mpl/selfupdate that referenced this issue Jan 16, 2025
@mpl
New option: run_once, and removed skip_waiting
df63163 
- new default behaviour: an update check is always attempted right from
  the start, regardless of check_frequency value.
- run_once: controls whether we exit right after the initial update
  check.
- skip_waiting: removed

Fixes gokrazy#6
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

New option: run_once, and removed skip_waiting mpl/selfupdate
3 participants
@stapelberg @mpl @damdo