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

Feat: Add direnv allow persistence between sessions #762

Closed
wants to merge 4 commits into from

Conversation

drmikecrowe
Copy link

@drmikecrowe drmikecrowe commented Jan 30, 2022

what

  • Enables persistence of direnv allow between sessions by dedicating $HOME/.geodesic/direnv to /usr/share/xdg_data_home/direnv in the container

why

  • direnv allow is not persisted between sessions

@drmikecrowe drmikecrowe requested a review from a team as a code owner January 30, 2022 03:39
@korenyoni korenyoni added the enhancement New feature or request label Jan 30, 2022
@korenyoni korenyoni changed the title fix direnv persistence Feat: Add direnv allow persistence between sessions Jan 30, 2022
Copy link
Member

@korenyoni korenyoni left a comment

Choose a reason for hiding this comment

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

Deferring to @Nuru because this makes the particular design decision to make use of ~/.geodesic/direnv for direnv allow persistence (high level wrapper functionality).

local geodesic_folder="$(dirname "$geodesic_default_env_file")"
local geodesic_direnv_folder=${geodesic_folder}/direnv
mkdir -p $geodesic_direnv_folder
DOCKER_ARGS+=(--volume="$geodesic_direnv_folder:/usr/share/xdg_data_home/direnv")
Copy link
Member

Choose a reason for hiding this comment

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

ENV XDG_DATA_HOME=/usr/share/xdg_data_home

Suggested change
DOCKER_ARGS+=(--volume="$geodesic_direnv_folder:/usr/share/xdg_data_home/direnv")
DOCKER_ARGS+=(--volume="$geodesic_direnv_folder:$XDG_DATA_HOME/direnv")

@@ -8,7 +8,7 @@ unset BASH_ENV
export DIRENV_LOG_FORMAT=

# Allow current working directory (if we have permission)
if [ -f .envrc ] && [ -w /etc/direnv/allow ]; then
if [ -f .envrc ] && [ -d /usr/share/xdg_data_home/direnv ] && [ -w /usr/share/xdg_data_home/direnv ]; then
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if [ -f .envrc ] && [ -d /usr/share/xdg_data_home/direnv ] && [ -w /usr/share/xdg_data_home/direnv ]; then
if [ -f .envrc ] && [ -d "${XDG_DATA_HOME}/direnv" ] && [ -w "${XDG_DATA_HOME}/direnv" ]; then

ENV XDG_DATA_HOME=/usr/share/xdg_data_home

@korenyoni
Copy link
Member

Edited PR description to match our pull request format.

@drmikecrowe
Copy link
Author

My problem is solved if I set DIRENV_ENABLED=true in my Dockerfile.

How should this be documented? Had I know this, none of this would be necesary

@korenyoni
Copy link
Member

My problem is solved if I set DIRENV_ENABLED=true in my Dockerfile.

How should this be documented? Had I know this, none of this would be necesary

I think we'll have to make a new place for it. At the very least a subsection in customization.md.

I've created #768

@drmikecrowe
Copy link
Author

I can document this, as I want to add some customization docs when I submit my fix for #594 (which I need some more consensus before I submit)

@Nuru Nuru added the do not merge Do not merge this PR, doing so would cause problems label Jan 31, 2022
@Nuru Nuru self-requested a review January 31, 2022 08:06
Copy link
Contributor

@Nuru Nuru left a comment

Choose a reason for hiding this comment

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

@drmikecrowe Thank you for this submission, and sorry for the confusion and the extra effort you had to go to find the solution.

We are not going to approve this PR for a bunch of reasons. Besides not being needed, if we wanted to persist the data on the host, we would want to have the option to do it on a per-image basis, for people that use more than one Geodesic-based Docker image. This would get pretty complicated, and it does not seem there is enough demand to justify the effort.

As for documentation (cc @korenyoni and #768), we currently document (to some extent) build-time configurations option in the Docker files. Specifically:

Granted, it would be better to have the options more fully documented, and consolidated in one place, and easier to find. We will work on that: #768

For those wondering about runtime configuration, while we do have good documentation on how to set runtime options, we unfortunately do not at the moment have much documentation on what the runtime options are. Your best resource for that at present is to review the code in the profile.d directory and look for all uppercase environment variables. We try to make them reasonably self-documenting, but of course we acknowledge we could do a lot better. We do try to document them pretty well when we first introduce them, so you may find better information in the release notes. See, for example "New options for customizing command line prompt appearance" in the Release Notes for v0.149.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
do not merge Do not merge this PR, doing so would cause problems enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants