Skip to content
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

fix: make direnv watch devbox.lock #2233

Merged
merged 1 commit into from
Aug 25, 2024
Merged

fix: make direnv watch devbox.lock #2233

merged 1 commit into from
Aug 25, 2024

Conversation

mjgallag
Copy link
Contributor

Summary

How was it tested?

Signed-off-by: Michael Gallagher <mjgallag@gmail.com>
@savil savil requested review from gcurtis and mikeland73 August 23, 2024 00:20
Copy link
Collaborator

@savil savil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! LGTM. Will just cross-check with others as well

@mikeland73
Copy link
Contributor

LGTM

@mjgallag
Copy link
Contributor Author

mjgallag commented Aug 23, 2024

Unfortunately because this change gets to .envrc via eval, and not a change to .envrc itself, it won’t bust direnv’s cache. Perhaps something to track in devbox.lock at some point?

@mikeland73
Copy link
Contributor

mikeland73 commented Aug 23, 2024

Unfortunately because this change gets to .envrc via eval, and not a change to .envrc itself, it won’t bust direnv’s cache. Perhaps something to track in devbox.lock at some point?

@mjgallag which cache is this? Does direnv track which files it watches and doesn't add new ones if . envrc doesn't change?

Our goal was to generate .envrc once and avoid changing it (I think it has to be approved every time it changes?). We could add a version to the generated .envrc file to force direnv to pick up the changes. (and detect a version missmatch)

@mjgallag
Copy link
Contributor Author

mjgallag commented Aug 23, 2024

@mikeland73 tough call on the best flow. I think putting a version in the generated .envrc would mean devbox users would have to upgrade to a new version of devbox and then be told (in release notes ... who reads those) to run generate again in order to get the change. And you do need to allow again when it changes (which generate would do). If change is shipped as is (please test ha) it wouldn't take a effect immediately after devbox upgrade, it would only take effect if devbox.json changes, .envrc changes (and they run allow) or they cd out and back into the directory, so they'll get the fix eventually. direnv will cache between prompts because files it is watching have not changed. I believe it stores a checksum in DIRENV_WATCHES env var for each watched file so as not to have to recalculate every prompt. In any case not something that needs to be solved with this pull and maybe this won't change much anyway.

@mikeland73
Copy link
Contributor

@mjgallag got it. From:

it wouldn't take a effect immediately after devbox upgrade, it would only take effect if devbox.json changes, .envrc changes (and they run allow) or they cd out and back into the directory.

sounds like doing cd or opening a new terminal and cd'ing to project will cause direnv to pick up the new .envrc right? This sounds OK to me.

Will test and merge if everything looks good.

@mjgallag
Copy link
Contributor Author

@mikeland73 yeah correct, so given this file is unlikely to change often i agree its OK. Thanks for taking a look. This should mean after running devbox update next prompt has the new package versions, rather than having to cd in and out to get.

@savil
Copy link
Collaborator

savil commented Aug 24, 2024

While not perfect, it does sound like an improvement from the status quo so 👍

@mikeland73 mikeland73 merged commit 8d48e38 into jetify-com:main Aug 25, 2024
23 of 24 checks passed
@mjgallag mjgallag deleted the make-direnv-watch-devbox-lock branch September 11, 2024 22:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants