[env] Fixed env_from bug when it is called from a subdirectory

[mohsenari]

Summary

Fixes this bug:
https://github.com/jetify-com/devbox/pull/2174/files#r1659165116

How was it tested?

• use compiled devbox
• put export FOO = 'BBBAR' in .env file
• put the following in a devbox.json file

  {
    "packages": [],
    "shell": {
      "init_hook": [
        "echo 'Welcome to devbox!' > /dev/null"
      ],
      "scripts": {
        "test": [
          "echo $FOO"
        ]
      }
    },
    "env_from": ".env"
  }

• create a subdirectory besides devbox.json called test
• cd test && devbox run test

[mikeland73]

This breaks with absolute paths. I think we need to treat absolute and relative paths differently.

[mohsenari]

pushed a commit to handle this case.

[mikeland73]

What you want is:

envfileAbsPath := c.EnvFrom
if !filepath.IsAbs(c.EnvFrom) {
  envfileAbsPath = filepath.Join(c.AbsRootPath, c.EnvFrom)
}

[savil]

should this scenario be envFileAbsPath = c.EnvFrom?

if the user has set

// devbox.json
{
   "env_from": "/my/absolute/path/foo.env`
}

Then, I'd think we want to just obey the absolute path directive.

Perhaps we should error if the env_from path is absolute, because that would mess up any reproducibility of the devbox.json config. But I'll be open to just letting it fly. We can expect our users to use their judgement for when they want to make their config a bit less reproducible. For example, for the sake of some quick changes they may want to experiment with.

[mohsenari]

I agree with the absolute path erroring out case. Reproducibility should be considered here. I'll push a commit to change this case to an error.

[mikeland73]

We allow absolute paths for flakes and plugins. I don't think env should be different. Absolute paths can be important for reproducibility as well.

[mikeland73]

Thinking about it even more, I don't think relative paths are any more reproducible than absolute paths! relative paths are more reproducible only if .env file is in the repo, but in that case you are better off just adding to the env field in devbox.json

[mohsenari]

@mikeland73 the idea is to prevent unintended mistakes that comes with putting absolute paths. For example, if user A puts /Users/A/projects/Aproject/.env and user B has the repo clone in /Users/B/... it will break the reproducibility.
Do you think this is an acceptable risk and we should allow absolute path here?

[mikeland73]

2 reasons:

• erroring on abs path is not backward compatible. This reason alone is a blocker. path of least resistance is always to preserve backward compatibility, especially if it's easier!
• relative poths also allow you to make it non reproducible! ../path/to/my/.env would also live outside the repo. Or just .env when .env is in gititnore.

Why would folks use this feature with .env file that is checked in to their repo? (I guess some convenience maybe, but seems low value). The use case for this feature is for people to have different .env files, so by definition reproducibility is not as important.

[mikeland73]

I don't understand this logic at all. Why would the path of the config matter?

[mohsenari]

This was handling the case when env_from was absolute. Your suggestion below works too, it's just handling if abs() vs if !abs()

[mikeland73]

If env_from is absolute you don't need to change it at all.

[mohsenari]

yeah I agree, I overcomplicated the logic there.

[mikeland73]

What you want is:

envfileAbsPath := c.EnvFrom
if !filepath.IsAbs(c.EnvFrom) {
  envfileAbsPath = filepath.Join(c.AbsRootPath, c.EnvFrom)
}

[mikeland73]

NACK

breaking absolute paths would be non-backward compatible