[local-state] Local state and fish improvements #1765
Conversation
| @@ -0,0 +1,116 @@ | |||
| // Copyright 2023 Jetpack Technologies Inc and contributors. All rights reserved. | |||
There was a problem hiding this comment.
would be nice to git mv such files to preserve history and make reviewing the code changes easier
There was a problem hiding this comment.
github shows the file as deleted, but I split the PR into 2 commits. If you take a look a7160e3 it contains the changes to the file.
| DevboxVersion string `json:"devbox_version"` | ||
| // fish has different generated scripts so we need to recompute them if user | ||
| // changes shell. | ||
| IsFish bool `json:"is_fish"` |
There was a problem hiding this comment.
I think this is fine for now, because our code gen logic switches on IsFish
Ideally, we'd have a shell enum value here (ref. the enum in devbox/shell.go) so that this is slightly more generalizable.
| filesystemLock, err := readStateHashFile(args.ProjectDir) | ||
| if err != nil { | ||
| return false, err | ||
| } | ||
| newLock, err := getCurrentStateHash(args) | ||
| if err != nil { | ||
| return false, err | ||
| } | ||
|
|
||
| return *filesystemLock == *newLock, nil | ||
| } | ||
|
|
||
| func readStateHashFile(projectDir string) (*stateHashFile, error) { | ||
| lockFile := &stateHashFile{} | ||
| err := cuecfg.ParseFile(stateHashFilePath(projectDir), lockFile) | ||
| if errors.Is(err, fs.ErrNotExist) { | ||
| return lockFile, nil | ||
| } | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| return lockFile, nil |
There was a problem hiding this comment.
its confusing to call these lockfiles in the local variables. Why not stateHashFile and similar?
| return nil, err | ||
| } | ||
|
|
||
| newLock := &stateHashFile{ |
| } | ||
|
|
||
| func stateHashFilePath(projectDir string) string { | ||
| return filepath.Join(projectDir, ".devbox", "local.lock") |
There was a problem hiding this comment.
I find the .lock pretty confusing usage here. Lockfiles in my mind represent a need to ensure that some particular state can be reproduced as done with package.lock or devbox.lock.
This feels to be more of a local-state-cache-representation or .devbox/state_cache_key.json, or simply .devbox/cache_key.json
WDYT?
There was a problem hiding this comment.
for sure. I was not bold enough to change the name, but there's no downside really.
| @@ -110,6 +110,11 @@ func writeInitHookFile(devbox devboxer, body, tmpl, filename string) (err error) | |||
| } | |||
| defer script.Close() // best effort: close file | |||
|
|
|||
| if body == devconfig.DefaultInitHook || strings.TrimSpace(body) == "" { | |||
There was a problem hiding this comment.
Mind adding a comment to the effect of:
// skip adding init-hook recursion guard for the
// default hook or any empty hook
As is, not obvious why the template below is being skipped.
Will also be a good idea to comment somewhere in the template that it will be skipped for default or empty hooks, in case we later introduce some other logic for init-hooks in the template files.
465b143 to
a7160e3
Compare
## Summary I accidentally merged #1765 without committing the fixes requested in the PR. This PR contains them. ## How was it tested?
Suspect IssuesThis pull request was deployed and Sentry observed the following issues:
Did you find this useful? React with a 👍 or 👎 |
Summary
A few fish and local state improvements:
shellenv --init-hooksoutput on a different shell).stateHashinstead oflocalLock)stateHashfrom lock file. These are different and should not depend on each other. They are still in the same package, but we can change that in the future.statehash. As @gcurtis warned us many moons ago, interfaces like this make code harder to follow.How was it tested?
Tested empty/default init hooks not having recursion protection:
devbox init devbox run echo foo cat .devbox/gen/scripts/.hooks.shTested local state with fish