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

Simple change to support shell hooks #45

Closed
wants to merge 3 commits into from
Closed

Conversation

venkytv
Copy link

@venkytv venkytv commented Aug 31, 2022

Summary

A simple change to support custom shell hooks. This is something I often need and currently use direnv for, but I find it convenient to have the configuration within devbox.json.

Fully understand if you decide not to pull this in. :) This is just a very quick hack, and in general, JSON is not that great a fit for storing multi-line strings. But I find devbox very useful, and this feature makes it a bit more useful for me.

How was it tested?

go build
go test
devbox init
devbox shell

@loreto
Copy link
Contributor

loreto commented Aug 31, 2022

@venkytv Thanks for sending.

Could you help me understand the use-case a tiny bit more? An example devbox.json using the shell hook would be great.

On the multi-line strings: I've considered letting people pick the format of the config file (i.e. if instead of having a devbox.json you had a devbox.yaml or a devbox.toml, we would detect that and use that file instead). What are your thoughts on that idea?

@venkytv
Copy link
Author

venkytv commented Aug 31, 2022

@loreto Sure. Essentially, a dev environment for us also includes setting a few environment-specific parameters, typically environment variables. A sample devbox.json could look like this:

{
  "packages": [
    "python3"
  ],
  "shell-hook": "export MYPARAM1=something\nexport MYPARAM2=42"
}

So, a devbox shell would launch with these parameters already set for the environment.

$ devbox shell
$ echo $MYPARAM
something

(Just noticed #42 which is requesting something similar to this.)

I could use direnv for this of course, but having it in devbox.json makes it easier to distribute a single file for use across the team.

And yes, YAML especially would be perfect for this as it has multiple ways to represent multiline strings!

By the way, I noticed that I have corrupted the PR with some additional commits I pushed into my fork after I submitted this. Do let me know if you want to include this, and I will clean up the commits and re-submit it.

Thanks again for this very useful tool!

@gcurtis
Copy link
Collaborator

gcurtis commented Aug 31, 2022

Thanks @venkytv!

One thing we should be aware of (and somewhat related to #15) is that devbox currently runs two shells:

  1. nix-shell always launches bash, which is where the hooks run.
  2. From that shell, we launch whatever is in SHELL to try and preserve the user's preferred shell. This means that any aliases and unexported variables or functions set in the hooks won't persist into the new shell.

If we implement something like this, we'd probably want to run the hooks inside the 2nd shell (which should be possible after I finish the work in #15) or limit it to exported env vars to avoid confusion.

@loreto
Copy link
Contributor

loreto commented Aug 31, 2022

@venkytv One more question for you. I open to the hook idea – I think it can be useful for some advanced use cases.

That said, for setting env variables, I was also thinking that, if there's a .env file present in your directory, devbox could automatically use it when starting the shell. Thoughts on that approach vs the shell hook approach for the env variables use-case?

Copy link
Collaborator

@LucilleH LucilleH left a comment

Choose a reason for hiding this comment

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

I personally really like this idea since I need it too myself, and this is a great way to get some quick ENV, iterm display, aws credentials, etc config setup. Bravo! I'm also open to the .env idea, but I find that a bit restricting. Curious to hear your thoughts.

I wonder if for multi-lines shell hook, we recommend developers to put them in a script such as shell.sh and reference the script that way? So devbox.json may look like this

{
  "packages": [
    "python3"
  ],
  "shell-hook": "./shell.sh"
}

@@ -91,7 +91,8 @@ func (d *Devbox) Build(opts ...docker.BuildOptions) error {
// environment.
func (d *Devbox) Plan() *planner.BuildPlan {
basePlan := &planner.BuildPlan{
Packages: d.cfg.Packages,
Packages: d.cfg.Packages,
ShellHook: d.cfg.ShellHook,
Copy link
Collaborator

@LucilleH LucilleH Aug 31, 2022

Choose a reason for hiding this comment

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

I'm assuming ShellHook is only applicable on devbox shell command, and not devbox build?

Copy link
Contributor

Choose a reason for hiding this comment

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

@LucilleH Plan is currently called by both devbox shell and devbox build

Copy link
Collaborator

@LucilleH LucilleH Aug 31, 2022

Choose a reason for hiding this comment

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

@loreto sorry to clarify, will/should the shell hook be called in the Dockerfile as well? (I lean more towards .env in this case)

Copy link
Contributor

Choose a reason for hiding this comment

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

It won't run because there's nothing that executes it. This behaves as you want. I'm just saying the same struct that includes the hook is passed to both shell and build.

@venkytv
Copy link
Author

venkytv commented Aug 31, 2022

@venkytv One more question for you. I open to the hook idea – I think it can be useful for some advanced use cases.

That said, for setting env variables, I was also thinking that, if there's a .env file present in your directory, devbox could automatically use it when starting the shell. Thoughts on that approach vs the shell hook approach for the env variables use-case?

@loreto That would work too, but I prefer the idea of having one file that contains everything the environment needs. The instructions to use it would be as simple as "download this devbox.yaml file and run devbox shell". If the config refers to external files, it becomes a bit more complicated. "Download devbox.json and the right .env file for it, make sure they are in the same directory, and then run devbox shell". Just makes things a bit more fragile, IMO.

The same goes for including a separate file and calling that from the shell-hook... one more thing which could potentially go out of sync. I'd rather use a YAML format for the config instead and stick the entire shell-hook script inline inside it.

@loreto
Copy link
Contributor

loreto commented Sep 1, 2022

@venkytv We're gonna move forward with a feature for shell hooks as you requested. That said, I'm waiting for @gcurtis PR to land, as it might affect how we go about implementing that.

@venkytv
Copy link
Author

venkytv commented Sep 1, 2022

Awesome! Thank you.

@loreto
Copy link
Contributor

loreto commented Sep 9, 2022

@venkytv We have a new implementation of this functionality in PR #93

I'm gonna close this PR in favor of that one – that said, hook functionality coming soon!

@loreto loreto closed this Sep 9, 2022
@Lagoja Lagoja added this to the 0.0.5 milestone Sep 9, 2022
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.

None yet

5 participants