central environment variable store

[EverlastingBugstopper]

This PR creates a centralized environment variable store that can be mocked in tests.

It defines an enum of all of the environment variables we use in rover in a single place, and also defines behavior for interacting with them.

benefits:

• all environment variables in one place!
  • easy to add a new one
  • same parsing logic everywhere
  • impossible to mistype APOLLO_ prefix
  • no ad-hoc std::env::var calls
• when tests are run, you can mock the environment variables instead of using the system's
  • means we can likely remove serial_test in most places where we had to add it because the system env var store would clobber the other tests
  • your dev environment won't cause side effects in your tests (i have failing tests because i have a global APOLLO_KEY variable set

drawbacks:

• makes it a bit more difficult to use environment variables in libraries like houston (gotta pass the parsed environment variables from the root of the app into our subcrates)
• adds some code which is always a liability

---

Tests are passing and no are no longer affected by environment variables on any individual developer's machine. Any new tests should follow the patterns here when testing code that relies on environment variables.

[EverlastingBugstopper]

hey @lrlna - I'd love your perspective on this before I spend some more time on it. The remaining work is just to actually use this central environment variable store everywhere we use environment variables. Not sure if you had thoughts/have seen other patterns that may be better for our use case.

[lrlna]

@EverlastingBugstopper I think to give you a better perspective, I'd like to get a few clarifications:

1. Could you elaborate a little bit more on this point in the PR's description:

makes it a bit more difficult to use environment variables in libraries like houston (this is what im stuck on right now)

2. Is env var use solely for Rover's internals, or do we expect our users to be able to insert and get them? I think this makes a very large difference in thinking about this API.

3. Would we want to persist env vars or make sure they are started anew for every terminal session? I know lots of folks configure their terminals to specifically persist env vars, so that's something to consider when making a decision on this.

4. How do you distinguish between the .config dir introduced in this PR and env?

[EverlastingBugstopper]

Could you elaborate a little bit more on this point in the PR's description:
makes it a bit more difficult to use environment variables in libraries like houston (this is what im stuck on right now)

yeah! so before this PR, houston just directly called std::env::var directly, but since this moves all the environment variable interactions to rover, these values must be passed to houston from rover, which just makes things a little bit more difficult (but not impossible!)

Is env var use solely for Rover's internals, or do we expect our users to be able to insert and get them? I think this makes a very large difference in thinking about this API.

all of these environment variables can and should be documented for external use

Would we want to persist env vars or make sure they are started anew for every terminal session? I know lots of folks configure their terminals to specifically persist env vars, so that's something to consider when making a decision on this.

the changes here should be purely related to where in the code the environment variables values are retrieved and shouldn't actually change behavior. that being said, we do want to support this use case! folks should be able to set these environment variables and have them persist.

How do you distinguish between the .config dir introduced in this PR and env?

so the .config dir is not actually introduced in this PR, this has been around since before I actually started working on Rover! It may be worth revisiting that, but generally the logic and behavior should not be changed at all with this PR

[lrlna]

as discussed outside of this issue:

I like having the env var setup as a separate module, especially since it makes testing a whole lot easier.

To be able to also use it in houston, it might be easier to have rover-env as a separate crate in cargo worskpaces so it can be easily accessed by all modules.

As for config, it's beyond the scope of this PR, and we ought to discuss it separately as not to block the good progress being done here!

[EverlastingBugstopper]

To be able to also use it in houston, it might be easier to have rover-env as a separate crate in cargo worskpaces so it can be easily accessed by all modules.

it's all coming back to me now..

so the problem with the approach i have right now is that it requires you to create and persist an instance of the RoverEnv struct. This struct is to be created once per invocation and then all environment variables are to be accessed through it (tests use a hashmap, the binary uses your global environment store).

the problem with this is that we have to start passing the values around everywhere, so it's not as easy as std::env::var("APOLLO_CONFIG_HOME") anymore. we create a new RoverEnv and store it in the Rover command line argument struct with let rover_env = RoverEnv::new(). This way we can then query it in each of the commands that Rover runs just by accessing it with self.rover_env.get. This makes it easy to access environment variables within Rover, but challenging when trying to query these environment variables in houston. This makes separating the module out into its own crate not actually that helpful to solve the immediate problem (though likely still useful overall). You still need to pass the instance of RoverEnv` from rover into each subcrate.

The current design of Houston makes this quite difficult as the functions in the impl Profile block do not take &self as a parameter, and are instead called with Profile:: syntax. I think this would all need to be overhauled in order to make everything work with RoverEnv and... it's proving itself difficult.

Perhaps there's another approach we can take here, or perhaps an easier way of getting the value of APOLLO_CONFIG_HOME shoehorned into houston. Any ideas here are greatly appreciated.

[lrlna]

This makes it easy to access environment variables within Rover, but challenging when trying to query these environment variables in houston.

This sentence got me thinking that perhaps we need to take step back. Could you walk me through what exactly your goal is with env vars? You previously mentioned that you wanted an easier testing scenario than you have now. Is there more?

In the meantime, here is how my previous teams addressed the env var / any other setup scenarios in CLIs. I usually try to use this sort of design, as it's easily testable:

The main entry point to the CLI handles all possible parsing and env var storage. Depending on the structure, and if statefullness is required, the main entry point will also initiate and keep track of state. This means that all env vars are initiated and written there as well. All "child" components, other initiated modules take either state, env vars, or configuration as parameters. Since the components/modules are independent of outside variables (forces? :D), it becomes a lot easier to test and to maintain. It's also a lot easier to spot regressions (at least in my experience). It does create an architecture that is reliant on a main entry point / "parent" module, but usually with CLI's I find that quite acceptable.

(This would be easier to understand with a diagram perhaps, but I lost my apple pencil and a new one comes tomorrow. Lmk, and I can draw this up instead.)

I do realise this probably requires a bit of an overhaul of how Rover does things now, but I want to put it out there as something we may wish to consider if we run into more issues with configuration / env vars.

[EverlastingBugstopper]

Yes - that makes a lot of sense. The parent - child relationship here is what I've started working on, I think it's just a matter of redesigning houston a bit to take those parameters. What you've described is pretty much the exact architecture I have here, I think it's just a matter of bending houston to match the new pattern. Should be ok with some heads down time.

[lrlna]

Cool, good to know. Also: happy to pair on this if you like!

…

On Thu, 10 Dec 2020, 18:52 Avery Harnish, ***@***.***> wrote: Yes - that makes a lot of sense. The parent - child relationship here is what I've started working on, I think it's just a matter of redesigning houston a bit to take those parameters. What you've described is pretty much the exact architecture I have here, I think it's just a matter of bending houston to match the new pattern. Should be ok with some heads down time. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#75 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AB53OCE224KVKV4YE4LZC43SUEDGZANCNFSM4TQ7CZDA> .

[EverlastingBugstopper]

This is ready for review now!

[lrlna]

Really really nice!

[EverlastingBugstopper]

comments addressed! thanks for the thorough review @lrlna 🥇