-
Notifications
You must be signed in to change notification settings - Fork 85
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
Limit update checks to once per day #2918
Conversation
cmd/cli/root.go
Outdated
util.Fatal(cmd, fmt.Errorf("failed to initialize bacalhau repo at '%s': %w", repoDir, err), 1) | ||
} else { | ||
ctx = context.WithValue(ctx, util.FSRepoKey, fsRepo) |
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.
We need an alternative to embedding things in the context. This pattern isn't sustainable, and documentation states it should be avoided. https://pkg.go.dev/context
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.
Use of context is a bit confusing because it looks like a different context references the repo, but I'm assuming there's some hidden pointer refs going on in the background.
versions, err := GetAllVersions(ctx) | ||
if err == nil { | ||
printMessage = &versions.UpdateMessage | ||
err = writeNewLastCheckTime(ctx) | ||
log.Ctx(ctx).Debug().Err(err).Str("LatestVersion", versions.LatestVersion.GitVersion).Msg("Performed update check") | ||
} | ||
}(cmd.Context()) |
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.
Is this (cmd.Context()) the ctx with the value attached in root.go?
ctx = cmd.Context()
...
ctx = context.WithValue(ctx, util.FSRepoKey, fsRepo)
It's kinda confusing that this works calling .Context()
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, there is a cmd.SetContext()
call that overlays the new context into the command before the end of the pre-run hook.
@@ -65,23 +69,82 @@ func GetAllVersions(ctx context.Context) (Versions, error) { | |||
|
|||
var printMessage *string = nil | |||
|
|||
const updateStatePath string = "update.json" | |||
const updateFrequency time.Duration = 24 * time.Hour | |||
|
|||
// StartUpdateCheck is a Cobra pre run hook to run an update check in the | |||
// background. There should be no output if the check fails or the context is | |||
// cancelled before the check can complete. | |||
func StartUpdateCheck(cmd *cobra.Command, args []string) { |
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.
We could pass the repo path as an argument to this method and remove the method GetFSRepo
which looks in the context.
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.
Alternatively, if that is not possible, lets un-export the method GetFSRepo
and it's internals as the pattern shouldn't be used outside this specific case.
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.
The challenge is that this method is implementing the pre/post-run hooks interface, which only accepts string arguments and a context. AFAICS the context is the only way to pass state into hooks, without using a global variable which I think would be worse.
I think un-exporting the method is a sensbile change, and I'll add a comment to the effect of "don't start using this because context is intended to be used sparingly".
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.
It turns out using the config system properly removes the need to do any of this, as it will now set up the absolute path on repo initialisation rather than exposing the repo here. Nice.
cmd/util/version.go
Outdated
const updateStatePath string = "update.json" | ||
const updateFrequency time.Duration = 24 * time.Hour |
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 would have expected values like these to go in the config pkg and file, but that can always comes later.
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, that's a good point and would be in keeping with the other repo values.
Is there any way to provide default values for these? As users upgrading won't have them in their config file ofc. If I just put them in the configenv
structs, will they get used if the value is missing from the config file?
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.
left some suggestions for alternatives.
We currently have a
We are now introducing a new |
I'm not sure... this is essentially moving a key-value store implemented on the filesystem into a key-value store implemented in a single file. We'd just be moving "mess on the filesystem" into "mess in a yaml file" and I'm not sure if I think that being conservative on files on the filesystem is all that valuable? What problems do we think this is going to cause as it scales? |
In #2893 we made more commands check for updates. Strictly not all of these checks are necessary as the code is unlikely to have changed regularly. Instead we should limit the amount we check for updates to (perhaps) once per day. Every time we perform an update check, we write the current time into update.json. Every time we run a command. if the current time is less than 24h after this value, don't do any checking. If the current time is 24h ahead of this value, do an update check.
c239cee
to
3f37a77
Compare
OK! Thanks for the comments everyone. I have now properly understood the config system and using it properly has actually made most of the issues around contexts disappear. So thanks for the push back @frrist! |
Through proper use of the config system, many of the code review complaints can be avoided entirely.
3f37a77
to
441d2f2
Compare
Just avoiding a bloated repo. No performance or scale issues, just a user experience issue when they see tens of files in their repo not knowing which ones they should or shouldn't touch. Shouldn't be a blocker for this PR, and we can make a more informed decision when we see more of these configs and states in the future. A slight improvement might be to make the file hidden. |
In #2893 we made more commands check for updates. Strictly not all of these checks are necessary as the code is unlikely to have changed regularly. Instead we should limit the amount we check for updates to (perhaps) once per day.
Every time we perform an update check, we write the current time into update.json. Every time we run a command. if the current time is less than 24h after this value, don't do any checking. If the current time is 24h ahead of this value, do an update check.
Resolves #2894.