SimDem2 Feature: Add ability to inject environment variables

[lastcoolnameleft]

To make the scripts more generic, SimDem should allow the ability to inject environment variables. I envision two recommended approaches:

• Specify a .env file to exec into the shell prior to start

  FOO=bar

• Allow to inject via CLI. (-e FOO=bar)

Ideally, both should be allowed with the CLI winning over the env file.

[SorraTheOrc]

SimDem already has this feature, see env.json - documented in variables

The current implementation uses JSON for convenience in coding, but it should be replaced with a more BASH like file as you suggest.

You can also inject variables from the shell environment as they are copied into the SimDem environment, however, this requires one to have them in the shell from which you execute the SimDem command. It would be nice to be able to inject them from the CLI, but not necessary (env.json has worked really well for me to date).

Finally, variables can be defined during execution. If SimDem comes across a variable that is not currently defined it will prompt the user for a value. However, when running in an unattended mode (e.g. test) this will fail, so it becomes necessary to have a setup.md to create sensible default values.

[lastcoolnameleft]

This has been implemented as a CLI option -s to specify a setup-script. See https://github.com/Azure/simdem/blob/simdem2/examples/setup-script/README.md for an example.

And here for docs: https://github.com/Azure/simdem/blob/simdem2/docs/feature_advanced.md#setup-script

Closing for now. Please re-open if there's any additional issues that need to be addressed for it.

[SorraTheOrc]

What you've implemented is nothing like the proposal in this thread. Which was to provide an .env file (ass supported in SimDem1 and should be supported in SimDem2) or provide a command line argument to supply environment variables.

What you implemented is the ability to execute a script ahead of SimDem. I don't see the value in this, but I do see a number of downsides.

The reason I see no value is that the exactly the same behaviour can be gained by executing a SimDem preqrequiste that does the necessary setup. This is then self documenting as it appears in a perrequesities. See, for example, https://github.com/Azure/Moodle/blob/master/docs/Environment-Variables.md which is a prereq for all other documents in that folder.

This ability to provide a seperate script that the user somehow "needs to know" they should execute it breaks one of the fundamental design goals of SimDem - that documentation is executable. This goes to the point of installing necessary software components too, for example see https://github.com/Azure/Moodle/blob/master/docs/Preparation.md from the same documentation set as the above example.

Am I missing something about the goal here?

[lastcoolnameleft]

As currently implemented, the feature from the commit above supports the fix of this issue and increases it's usability with less code. If desired, a user can specify a .env file as such:

foo.env

FOO=bar
BAR=$FOO

and call it the same way:

simdem -s foo.env README.md

It also has the following benefits:

• Less code
  • HUGE WIN Just had to tie in executor.run_cmd().
  • This streamline will be useful if/when we support Powershell
• More flexibility
  • Can specify .env or .sh files
  • Allows for dynamically allocated env vars
  • env.json is hardcoded
  • Prerequisite assumes one size fits all. This enables unique environments, which I believe will be important as SimDem scripts are shared.

For example, if using secrets in one SimDem document, a CI/CD pipeline could use Azure Managed Service Identity, but a demo/tutorial could use Key Vault, OSX Keychain, etc.

In short, the purpose of an env file is to allow for easy injection of env vars. This supports that feature, but also allows for more flexibility with less code.

[SorraTheOrc]

I agree with the goal for env variables (the topic of this issue). I don't agree with the implementation.

What is wrong with the implementation in SimDem1 as linked to in my first comment? This is much more flexible than the -s option you have added as it allows multiple levels of definition file, e.g. put sensible defaults in "env.json" and local secrets in "env.local.json" (not objecting to the format change, I prefer yours for this).

To look at your benefits...

Less code - in terms of LOC the code to automatically look up default files is single or low double digit lines. SimDem1 does implement the ability to discover the config file in different places in the filesystem hierarchy, I'd support replacing this with a command line switch to tell SimDem where to look).

More flexibility - I do not support the provision of using .sh for the reasons expressed above (it conflicts with a basic design principle of SimDem).

More flexibility - sure allow a command line switch to overload the defaullt filename/location, that would be a good addiiton. But this should be in addition to the flexibility afforded by having executable prerequisites (equivalent to your .sh option) or global defaults or local values as implemented in the current SimDem

More Flexibility - prerequisites assumes one size fits all - I'd phrase this as the DRY principle (don't repeat yourself). SimDem1 allows a variable to be defined in env.json which will take priority over anything defined later in a .md file. Therefore it does not assume one size fits all. It assumes most things are fine as default values and allows configuration when that's not the case.

Your example of secrets in a CI/CD pipeline are fully supported in SimDem1. See https://github.com/Azure/acs-demos/blob/master/.circleci/config.yml where the same config files are used in circle CI, but with some values overridden by environment variables configured on the build machine. When these tests are run locally "az login" is used, in circle ci a service principle is used.

This results in far less configuration CODE. It is a single environment variable that needs to change, not a complete configuration file.

To be clear, I suppport the goal defined in the original issue:

Specify a .env file to exec into the shell prior to start
FOO=bar
Allow to inject via CLI. (-e FOO=bar)

The first option, as noted in my original comment, is implemented in SimDem1 and, in my opinion, is a superior solution to this. The addition of '-s' enhances that and as you note allows the reduction of code by removingt the lookup logic - that is have a single default location that is overridable by '-s'

This patch does not implement the '-e' option which is a shame, but not a blocker. In SimDem1 the entire local environment is copied into the SimDem environment so '-e' would just be a more flexible way of doing this.

The provision of using '.sh' breaks SimDem design. If it's there as a side effect I suppose I could live with it. But as a documented feature I think it is a mistake.

So, to be summarize, I'm:

+1 on a change to .env from "env.json" -1 on encouraging the use of .sh -0 on allowing the use of .sh -1 on the requirement to provide -s to discover a .env file -1 on the loss of the ability to have global .env and local .env (.local.env?)

Ross

[SorraTheOrc]

Accidentally closed.

[lastcoolnameleft]

I've been mulling this over for the past few days. Here's my thoughts:

RE: -1 on encouraging use of .sh

• We don't have to encourage the use of .sh. We could state it's for ENV vars, and let people implement it as they choose. The method for injecting env vars is the same as executing commands, so I don't see a reason for making it any different.

Re: -1 on the loss of the ability to have global .env and local .env (.local.env?)

• I'm against the global .env and local .env files. I believe it clutters the directory when one .env file should suffice.
  • That said, I'm open to be swayed otherwise.
  • Adding that feature wouldn't break any thing, but requires time to implement
  • Since you feel strongly about that feature, would you be willing to implement it? I'm more than happy to enable you for that.

Re: -1 on the requirement to provide -s to discover a .env file

• I've implemented the -e flag for injecting env variables. This is in the simdem2 branch now right now.

  ➜  simdem git:(simdem2) simdem -e FOO=bar /tmp/foo.md
  $ FOO=bar
  $ echo $FOO
  bar

NOTE:

• To make the options a bit more evident, I've moved some of the CLI options around. Notably:
  • -b for "bootstrap" (inject a .env/.sh file)
  • -e (Previously, -e was for the executor class, but I moved that to -s for "shell class"). Not it's for injecting env var via CLI (see example above)

[SorraTheOrc]

Agree using .sh for environment variable definition is OK, but then it duplicates the .env functionality.

I'm still -1 on the loss of local and global.env files. It's just good practice in my opinion. Consider a solution that needs an SSH key. The .env file is identical across all environments except the SSH key. Creating a local env file means that the global .env remains in version control while the SSH key is in the local one, which is excluded from version control. In this form there is now drift from my .env file and yours as we are both pushing into a shared code repo.

Your .sh solution doesn't support this, unless I override my command to include the SSH key - now my CLI command is different from yours and it is very cumbersome.

[lastcoolnameleft]

@rgardler I took a first-pass at implementing the env files, can you take a look at it: https://github.com/Azure/simdem/blob/simdem2/simdem/mode/common.py#L44

NOTE 1: Since we're moving away from a JSON file to a more flat file, I didn't know what to call it. (From env.json -> env.env sounded weird) So I currently have it look for a file called env.sh. I'm not tied to the name, so I'm open to suggestions.

NOTE 2: It displays the result of running the env file before running anything else. See: https://github.com/Azure/simdem/blob/simdem2/examples/environment-files/expected_result.tutorial

Example: https://github.com/Azure/simdem/tree/simdem2/examples/environment-files

Does this meet your expectations for injecting environment variables? Let me know if there's anything else I'm missing as it wasn't as bad to implement as I thought it might be.

[lastcoolnameleft]

Closing this issue. Please feel free to re-open or create a new issue if this does not meet needs.