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

Sourcing Environment Variables From Hooks #60

Closed
wants to merge 11 commits into from
Closed

Sourcing Environment Variables From Hooks #60

wants to merge 11 commits into from

Conversation

Shizcow
Copy link

@Shizcow Shizcow commented Mar 21, 2021

Closes #57

Description

This PR allows for setting environment variables within pre_deploy and pre_undeploy files, which will then be taken in for the current process.

Use Example

Say we have the following snippet within a global.toml file:

[default.files]
my_config_file = "$MAGIC_DIR/my_config_file"

Before this PR, one would need to export $MAGIC_DIR before running. Now, one can have the following pre_deploy.sh:

export MAGIC_DIR=~/target_magic_dir

This will now be shell expanded correctly.

Implementation

Modifications here are done as follow:

  1. Instead of parse and generate the config file in one step, do everything except shell expand
  2. Run pre_deploy, sourcing the contents
  3. Now shell expand file names from the config

The sourcing is roughly done as follows:

  1. Create a pipe to talk to the child shell, notated here as FD
  2. Wrap the sh call as sh -c "source pre_deploy.sh"; printenv -0 >&FD
  3. Inspect the read end of FD, parsing the output as environment variable pairs
  4. use std::env::set_var to forward the child's variables to us

TODO

This PR will be feature-complete when the following is done:

  • Make it work on unix
  • Make it work on windows
  • Write tests

I have yet to find a way to source environment variables within windows batch files. And given that I don't run windows, it will be a good bit of time before I were to write and test this on my own. Ideas on this point are welcome.

@SuperCuber SuperCuber marked this pull request as ready for review March 22, 2021 15:01
@SuperCuber
Copy link
Owner

Whoops, I'm kinda new to the UI of PRs hehe. Not sure how to undo this mark

@SuperCuber SuperCuber marked this pull request as draft March 22, 2021 15:09
src/hooks.rs Outdated Show resolved Hide resolved
@SuperCuber
Copy link
Owner

Looked through the code a bit, I don't really know much about pipes so I'm not sure if this is the best way to implement this.

This is definitely on the path to getting merged - figuring out windows support (or deciding against it) is an important point though.

TIL you can drop values like this -- much cleaner.

Co-authored-by: SuperCuber <hubtig@amitgold.com>
@Shizcow
Copy link
Author

Shizcow commented Mar 24, 2021

I'm not sure if this is the best way to implement this

Out of all the inter-process communication methods, pipes tick quite a few boxes for being useful here:

  • Available natively in sh (this is the biggest point)
  • Doesn't require compiling a companion executable
  • Don't need to worry about the filesystem (what if /tmp/ is full?)
  • POSIX compliance (should work OOTB on macos)
    • Also has a Windows analogue (less code duplication and testing)
  • Has somewhat decent Rust support (compared to, say, shared memory)
  • Fairly light-weight, doesn't add a ton of slow-compiling dependencies to dotter

Of course, not trying to say that pipes are the be-all-end-all. I would like to at least move away from anonymous pipes and into named pipes; this would resolve the issue of a script using FD3 as a hard-coded value (it's not too uncommon). The interprocess crate has some support for this, and it seems to be somewhat system-independent.

I'll do a bit of research on this front and report back / update implementation if I find compelling evidence for using any particular communication method. Open to suggestions.

@Shizcow
Copy link
Author

Shizcow commented Apr 25, 2021

Sorry for no update on this in a long time... school is sucking up all of my time.

I've been thinking about this a little bit more. Is there any reason not to just have additional shell expansion syntax? I don't see why name = "~/$(magic_dir_command)/name" would be an issue. It would require more Windows/Unix considerations and figuring out what characters need escaped, but it would be much easier/cleaner to implement. However, the difference in syntax between batch and sh may cause considerable issues. Maybe require keys that use command substitution to declare an OS? Just food for thought.

@SuperCuber
Copy link
Owner

This is already kinda planned in #61 - if we add full handlebars rendering, then you can use the command_output helper for this.

Now that I'm thinking about it, #61 would also make the functionality in #52 redundant - since you can just make the path render to an empty string to disable the file.


I'm not sure however how this addresses the use case in the pull request though... Maybe I'm misunderstanding something.

@SuperCuber
Copy link
Owner

Hey, the CI went through some changes to make sure it works for older versions of Linux. When/if you continue working on this make sure you merge the changes from master to your branch and re-run the CI (I might need to re-approve it)

@lasse16 lasse16 mentioned this pull request May 29, 2021
@Shizcow
Copy link
Author

Shizcow commented Jun 3, 2021

With recent Firefox updates, I no longer need this for the exact use-case that I'm after. While I still think this may be useful, I am no longer interested in putting in work towards the solution.

@Shizcow Shizcow closed this Jun 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[FEATURE] Inherenting pre_deploy.sh environment variables
2 participants