[Avocado-devel] [RFC] Environment Variables

Wed Jun 1 18:07:00 UTC 2016

On Tue, May 31, 2016 at 07:30:43AM -0300, Cleber Rosa wrote:
> 

I'm replying on top of Cleber because he already said a few
things I was going to say.

> On 05/25/2016 05:31 AM, Amador Pahim wrote:
> > Hi folks,
> > 
> > We have requests to handle the environment variables that we can set to
> > the tests. This is the RFC in that regard, with a summary of the ideas
> > already exposed in the original request and some additional planning.
> > 
> > The original request is here:
> > https://trello.com/c/Ddcly0oG/312-mechanism-to-provide-environment-variables-to-tests-run-on-a-virtual-machine-remote
> > 
> > 
> > Motivation
> > ==========
> > Avocado tests are executed in a fork process or even in a remote
> > machine. Regardless the fact that Avocado is hard coded to set some
> > environment variables, they are for internal consumption and user is not
> > allowed to control/configure its behavior.
> 
> You mean this:
> 
> http://avocado-framework.readthedocs.io/en/latest/WritingTests.html#environment-variables-for-simple-tests
> 
> Right? Basically, the fact that Avocado sets some of the job/test state as
> environment variables, that can be used by SIMPLE tests.
> 
> > The motivation is the request to provide users an interface to set
> > and/or keep environment variables for test consumption.

I'm not sure if they're necessarily for test consumption. I think
the motivation for the original request was to provide the
standard Unix interface of environment variables for when tests
are run remotely.

These environment variables can change the behavior of both
Avocado (the runner itself), the tests (after all nothing
prevents the test writer from using them) and all sub-processes
executed by the test.

Locally, this is standard:

  $ TMPDIR=/whatever/tmp VAR=foo ./avocado run test1.py

But when running avocado remotely, there's no way to configure
the environment in the destination. The environment variables set
in the command line below will not be "forwarded" to the remote
environment:

  $ TMPDIR=/whatever/tmp VAR=foo ./avocado run test1.py \
                                 --remote...

> > 
> > Use cases
> > =========
> > 1) Use the command line or the config file to set the environment
> > variables in tests processes environment; access those variables from
> > inside the test.
> > 2) Copy from current environment some environment variable(s) to the
> > tests processes environment; access those variables from inside the test.

I think we don't even have to go that far. We can simply say the
intention is to set the environment variables in the environment
where Avocado is run. The mechanism is quite standard and well
understood.

And here comes an important point: I don't think this should be a
mechanism to pass variables to tests. Although, again,
environment variables can be used for that purpose, Avocado
should have a proper interface to provide a dictionary of
configuration and variables to each test.

Currently, this is erroneously provided by the multiplexer
(including --mux-inject), but it should be cleaned up in the near
future.

> > 
> > Proposal
> > ========
> > - To create a command line option, under the `run` command, to set
> > environment variables that will be available in tests environment process:
> > 
> >  $ avocado run --test-env='FOO=BAR,FOO1=BAR1' passtest.py
> > 
> 
> I can relate to this use case...

This would be a simple way of doing it, but something like
"--env-keep=FOO,FOO1" could be a better approach (more about it
below). So one would write it this way:

  $ FOO=BAR FOO1=BAR1 avocado run --env-keep='FOO,FOO1' passtest.py

> 
> > - To create an option in config file with a dictionary of environment
> > variables to set in test process environment. It can be used as a
> > replacement or complement to the command line option (with lower priority):
> > 
> >  [tests.env]
> >  test_env_vars = {'FOO': 'BAR', 'FOO1': 'BAR1'}
> > 
> 
> ... while putting those in a config file does not seem like something one
> would do.
> 
> In all cases, and more explicitly in the config file example, this is only
> really necessary if/when the environment variable to pass to the test
> actually harms Avocado (considering a local execution, that is, in a forked
> process).
> 
> So, if Avocado and the test, share the use of environment variables by the
> same name, then this is a must.  Also in the case of execution in other
> "runners", such as remote/vm, this can be quite valuable.

I don't like this interface because I think this opens the door
for abuse. Like I said in a previous paragraph, this interface
should not be a mechanism for passing variables to tests.

> 
> > - Create an option in config file with a list of environment variable
> > names to copy from avocado main process environment to the test process
> > environment (similar to env_keep in the /etc/sudoers file):
> > 
> >  [tests.env]
> >  env_keep = ['FOO', 'FOO1', 'FOO2']
> > 

I like this approach because it reinforces the message that we're
keeping (or forwarding) some of the environment variables from
the original environment where the test runner was run.

> > 
> 
> Right, this makes sense. But it also brings the point that we may actually
> change the default behavior of keeping environment variables from Avocado in
> the tests' process.  That is, they would get a much cleaner environment by
> default.  While this sounds cleaner, it may break a lot of expectations.

I wonder what the motivation would be to clean-up the environment
where tests are run. Can you please elaborate?  If we indeed
decide to implement this change, then I would say we should honor
whatever is set in env_keep.

> 
> > For every configuration entry point, the setting have to be respected in
> > local and remote executions.
> > 
> > Drawbacks
> > =========
> > 
> > While setting an environment variable, user will be able to change the
> > behavior of a test and probably the behavior of Avocado itself. Maybe
> > even the OS behavior as well. We should:
> > - Warn users about the danger when using such options.
> 
> I fail to see where an environment variable, to be set by Avocado in the
> test process, can or should impact Avocado itself.  If it does, then we'd
> probably be doing something wrong.  I'm not sure we need warnings that
> exceed documenting the intended behavior.

I think the environment has to be set where Avocado is run, not
where tests are run. Which is why I prefer --env-keep and
[env-keep].

So in the case of:

  $ FOO=bla avocado run test.py --remote=...

$FOO is available inside the environment where Avocado and its
tests are run, both locally and remotely.

> 
> > - Protect Avocado environment variables from overwriting.
> 
> About protecting the Avocado's own environment variables: agreed.

This is something that won't need any change. There are variables
which are read by Avocado and others which are written by it.

For example: 

 * TMPDIR will influence Avocado's behavior (standard Unix
   variable)
 * AVOCADO_VERSION is written by Avocado. Setting it externally
   won't make any difference.

  $ TMPDIR=/home/ademar/tmp avocado run examples/tests/env_variables.sh --show-job-log | grep Temporary
  Temporary dir: /home/ademar/tmp/avocado_L9YiE4

  $ AVOCADO_VERSION=0 ./scripts/avocado run examples/tests/env_variables.sh --show-job-log | grep Version
  [stdout] Avocado Version: 35.0

> 
> > 
> > Looking forward to read your comments.
> > 
> 
> Overall, this is definitely welcome.  Let's discuss possible implementation
> issues, such as remote/vm support, because it wouldn't be nice to introduce
> something like this with too many caveats.
> 

We appear to have different understandings about what this
feature should be about. IMO it should be about the standard unix
environment where Avocado and tests are run. The primary use-case
is for remote/vm support (in other words, whenever there's a
change in the environment).

Thanks.
   - Ademar

-- 
Ademar Reis
Red Hat

^[:wq!