fix: allow setting MagicDir in Options #337
Conversation
envbuilder.go
Outdated
| // We always expect the magic directory to be set to the default, signifying that | ||
| // the user is running envbuilder in a container. | ||
| // If this is set to anything else we should bail out to prevent accidental data loss. | ||
| defaultMagicDir := constants.MagicDir("") | ||
| kanikoDir, ok := os.LookupEnv("KANIKO_DIR") | ||
| if !ok || strings.TrimSpace(kanikoDir) != constants.MagicDir { | ||
| if force { | ||
| bailoutSecs := 10 | ||
| logger(log.LevelWarn, "WARNING! BYPASSING SAFETY CHECK! THIS WILL DELETE YOUR ROOT FILESYSTEM!") | ||
| logger(log.LevelWarn, "You have %d seconds to bail out!", bailoutSecs) | ||
| for i := bailoutSecs; i > 0; i-- { | ||
| logger(log.LevelWarn, "%d...", i) | ||
| <-time.After(time.Second) | ||
| } | ||
| } else { | ||
| logger(log.LevelError, "KANIKO_DIR is not set to %s. Bailing!\n", constants.MagicDir) | ||
| if !ok || strings.TrimSpace(kanikoDir) != defaultMagicDir.String() { | ||
| if !force { | ||
| logger(log.LevelError, "KANIKO_DIR is not set to %s. Bailing!\n", defaultMagicDir.String()) | ||
| logger(log.LevelError, "To bypass this check, set FORCE_SAFE=true.") | ||
| return errors.New("safety check failed") | ||
| } | ||
| bailoutSecs := 10 | ||
| logger(log.LevelWarn, "WARNING! BYPASSING SAFETY CHECK! THIS WILL DELETE YOUR ROOT FILESYSTEM!") | ||
| logger(log.LevelWarn, "You have %d seconds to bail out!", bailoutSecs) | ||
| for i := bailoutSecs; i > 0; i-- { | ||
| logger(log.LevelWarn, "%d...", i) | ||
| <-time.After(time.Second) | ||
| } |
There was a problem hiding this comment.
review: inverted the logic here for readability.
| for k := range configFile.AuthConfigs { | ||
| logf(log.LevelInfo, "Docker config contains auth for registry %q", k) | ||
| } |
There was a problem hiding this comment.
review: I found this useful when troubleshooting!
options/options.go
Outdated
| // MagicDir is the path to the directory where all envbuilder files should be | ||
| // stored. By default, this is set to `/.envbuilder`. This is intentionally | ||
| // excluded from the CLI options. | ||
| MagicDir constants.MagicDir |
There was a problem hiding this comment.
review: you can now set MagicDir in Options. You can't set it through ENVBUILDER_MAGIC_DIR though; this is intentional.
constants/constants.go
Outdated
| return defaultMagicDir | ||
| } | ||
| return filepath.Join("/", string(m)) | ||
| } |
There was a problem hiding this comment.
I'm thinking this abstraction could be a source of bugs. One might not always think to call String and instead do string(MagicDir) which would result in an empty string or potentially lack of /.
I'd rather see this as a validation step in options so that the input is verified there, and an error can be returned if it's not absolute. The options struct could have the MagicImage and MagicBuilt functions if that's preferable.
There was a problem hiding this comment.
I think a simpler option here it to simply change the type to a struct with a single unexported field. I'm less worried about leaking the implementation details; we can simply move MagicDir to an internal package.
Co-authored-by: Danny Kopping <[email protected]>
Co-authored-by: Mathias Fredriksson <[email protected]>
I moved some of the logic to options, and the rest to |
- Unexports constants.MagicDir and moves it to an internal package. - Moves other const/var declarations in `constants` package to where they are used - Removes constants package - Adds MagicDirBase to options (intentionally not configurable via CLI) - Removes usages of default MagicDir to instead use path provided by options Co-authored-by: Danny Kopping <[email protected]> Co-authored-by: Mathias Fredriksson <[email protected]> (cherry picked from commit e6c8c66)
Relates to coder/terraform-provider-envbuilder#43
This PR unexports
constants.MagicDirand replaces it with a different implementation that supports dynamically changingMagicDirat runtime.There is also some more "strategic" hard-coding of certain values to ensure they do not change when
MagicDirchanges.See also: #336
A subsequent PR will update the Terraform provider.