-
Notifications
You must be signed in to change notification settings - Fork 270
[local-state] Local state and fish improvements #1765
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,116 @@ | ||
// Copyright 2023 Jetpack Technologies Inc and contributors. All rights reserved. | ||
// Use of this source code is governed by the license in the LICENSE file. | ||
|
||
package lock | ||
|
||
import ( | ||
"errors" | ||
"io/fs" | ||
"path/filepath" | ||
|
||
"go.jetpack.io/devbox/internal/build" | ||
"go.jetpack.io/devbox/internal/cachehash" | ||
"go.jetpack.io/devbox/internal/cuecfg" | ||
) | ||
|
||
// stateHashFile is a non-shared lock file that helps track the state of the | ||
// local devbox environment. It contains hashes that may not be the same across | ||
// machines (e.g. manifest hash). | ||
// When we do implement a shared lock file, it may contain some shared fields | ||
// with this one but not all. | ||
type stateHashFile struct { | ||
ConfigHash string `json:"config_hash"` | ||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this is fine for now, because our code gen logic switches on Ideally, we'd have a shell enum value here (ref. the enum in |
||
LockFileHash string `json:"lock_file_hash"` | ||
NixPrintDevEnvHash string `json:"nix_print_dev_env_hash"` | ||
NixProfileManifestHash string `json:"nix_profile_manifest_hash"` | ||
} | ||
|
||
type UpdateStateHashFileArgs struct { | ||
ProjectDir string | ||
ConfigHash string | ||
// IsFish is an arg because in the future we may allow the user | ||
// to specify shell in devbox.json which should be passed in here. | ||
IsFish bool | ||
} | ||
|
||
func UpdateAndSaveStateHashFile(args UpdateStateHashFileArgs) error { | ||
newLock, err := getCurrentStateHash(args) | ||
if err != nil { | ||
return err | ||
} | ||
|
||
return cuecfg.WriteFile(stateHashFilePath(args.ProjectDir), newLock) | ||
} | ||
|
||
func isStateUpToDate(args UpdateStateHashFileArgs) (bool, error) { | ||
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 | ||
Comment on lines
+50
to
+71
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. its confusing to call these lockfiles in the local variables. Why not There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. will change |
||
} | ||
|
||
func getCurrentStateHash(args UpdateStateHashFileArgs) (*stateHashFile, error) { | ||
nixHash, err := manifestHash(args.ProjectDir) | ||
if err != nil { | ||
return nil, err | ||
} | ||
|
||
printDevEnvCacheHash, err := printDevEnvCacheHash(args.ProjectDir) | ||
if err != nil { | ||
return nil, err | ||
} | ||
|
||
lockfileHash, err := getLockfileHash(args.ProjectDir) | ||
if err != nil { | ||
return nil, err | ||
} | ||
|
||
newLock := &stateHashFile{ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
ConfigHash: args.ConfigHash, | ||
DevboxVersion: build.Version, | ||
IsFish: args.IsFish, | ||
LockFileHash: lockfileHash, | ||
NixPrintDevEnvHash: printDevEnvCacheHash, | ||
NixProfileManifestHash: nixHash, | ||
} | ||
|
||
return newLock, nil | ||
} | ||
|
||
func stateHashFilePath(projectDir string) string { | ||
return filepath.Join(projectDir, ".devbox", "local.lock") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I find the This feels to be more of a local-state-cache-representation or WDYT? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. for sure. I was not bold enough to change the name, but there's no downside really. |
||
} | ||
|
||
func manifestHash(profileDir string) (string, error) { | ||
return cachehash.JSONFile(filepath.Join(profileDir, ".devbox/nix/profile/default/manifest.json")) | ||
} | ||
|
||
func printDevEnvCacheHash(profileDir string) (string, error) { | ||
return cachehash.JSONFile(filepath.Join(profileDir, ".devbox/.nix-print-dev-env-cache")) | ||
} | ||
|
||
func getLockfileHash(projectDir string) (string, error) { | ||
return cachehash.JSONFile(lockFilePath(projectDir)) | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Mind adding a comment to the effect of:
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. |
||
_, err = script.WriteString(body) | ||
return errors.WithStack(err) | ||
} | ||
|
||
t, err := template.New(filename).Parse(tmpl) | ||
if err != nil { | ||
return errors.WithStack(err) | ||
|
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.
would be nice to
git mv
such files to preserve history and make reviewing the code changes easierThere 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.
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.