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

[core] Add explicit, testing-only fs root support. #3030

Merged
merged 1 commit into from Aug 29, 2017

Conversation

fnichol
Copy link
Collaborator

@fnichol fnichol commented Aug 29, 2017

This change reverts the behvaior change of the FS_ROOT environment
variable in #2729 and introduces a new, testing only environment
variable to manipulate the filesystem root when being used in
integration and functional testing.

In the Habitat codebase we try to discourage reading environment
variables inside library code which could affect a program's behavior at
runtime. Instead, we highly encourage reading and dealing with
environment variables during the CLI argument parsing phase of a program
and using explicit Rust types when passing data into inner libraries.
Note that there can still be default-generating functions which could
read from an environment variable, but the calling sites for these
functions should still be at a program CLI parsing phase.

In order to better support running the Supervisor on Windows in a Studio
environment, we relax this rule due to the behavior of Windows path and
PowerShell filesystem mounts. We hope that this is temporary and will be
resolved once we add better Studio isolation on Windows. The non-Windows
platforms however remain unchanged.

There is a very limited use case for an FS_ROOT environment variable
which only the hab CLI uses for certain subcommands including hab pkg install and hab pkg binlink. This is present to support use cases
where a Habitat user may need to install packages on a mounted volume
that will become the bootable, root filesystem at a later point in time
(think operating system installers). The Supervisor and most other hab
CLI subcommands don't honor this environment variable because the
internal linking in most Habitat packages will not support running
packages out of another directory (linked libraries in Habitat are
linked via absolute pathing to maximize portability and correct behavior).

Recently, some excellent work was done to improve testing of the
Supervisor as a black box program. Some of the tests only require
minimal packages with no executing code and could therefore be run out
of temporary directories, and so these tests used the FS_ROOT
environment variable to manipulate the root of all Habitat paths.

This change preserves the strategy of these tests but rather introduces
a test-only environment variable which the test suite can set, knowing
what the consequences will be (see above). Additionally, if this
environment variable is used, a message will be printed to the standard
error stream notifying any user that this is intended only for testing
and not production. Note that by default, the test suites have their
stdout/stderr streams redirected and so this warning wouldn't appear
unless the --nocapture flag is added. This is one instance where the
rule above ("don't honor environment varibles in inner library code")
isn't an unbreakable law, it just needs to be carefully considered.

tenor-59160273

@thesentinels
Copy link
Contributor

Thanks for the pull request! Here is what will happen next:

  1. Your PR will be reviewed by the maintainers
  2. If everything looks good, one of them will approve it, and your PR will be merged.

Thank you for contributing!

@fnichol fnichol force-pushed the fnichol/explicit-testing-fs-root branch from fed1da1 to bdf4d24 Compare August 29, 2017 18:32
Copy link
Contributor

@christophermaier christophermaier left a comment

Choose a reason for hiding this comment

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

Looks great! Feel free to approve once the conflict is resolved.

@fnichol fnichol force-pushed the fnichol/explicit-testing-fs-root branch from bdf4d24 to 90503bd Compare August 29, 2017 20:15
@fnichol
Copy link
Collaborator Author

fnichol commented Aug 29, 2017

Okay, I think I fixed a thing here? It's getting confusing with other noise. In short:

@fnichol
Copy link
Collaborator Author

fnichol commented Aug 29, 2017

Yep, right on the money…

@fnichol
Copy link
Collaborator Author

fnichol commented Aug 29, 2017

Pretty sure #3037 is going to clear this failure up, then we can merge once rebased and green.

This change reverts the behvaior change of the `FS_ROOT` environment
variable in #2729 and introduces a new, testing only environment
variable to manipulate the filesystem root when being used in
integration and functional testing.

In the Habitat codebase we try to discourage reading environment
variables inside library code which could affect a program's behavior at
runtime. Instead, we highly encourage reading and dealing with
environment variables during the CLI argument parsing phase of a program
and using explicit Rust types when passing data into inner libraries.
Note that there can still be default-generating functions which could
read from an environment variable, but the calling sites for these
functions should still be at a program CLI parsing phase.

In order to better support running the Supervisor on Windows in a Studio
environment, we relax this rule due to the behavior of Windows path and
PowerShell filesystem mounts. We hope that this is temporary and will be
resolved once we add better Studio isolation on Windows. The non-Windows
platforms however remain unchanged.

There is a very limited use case for an `FS_ROOT` environment variable
which only the `hab` CLI uses for certain subcommands including `hab pkg
install` and `hab pkg binlink`. This is present to support use cases
where a Habitat user may need to install packages on a mounted volume
that will become the bootable, root filesystem at a later point in time
(think operating system installers). The Supervisor and most other `hab`
CLI subcommands don't honor this environment variable because the
internal linking in most Habitat packages will not support running
packages out of another directory (linked libraries in Habitat are
linked via absolute pathing to maximize portability and correct behavior).

Recently, some excellent work was done to improve testing of the
Supervisor as a black box program. Some of the tests only require
minimal packages with no executing code and could therefore be run out
of temporary directories, and so these tests used the `FS_ROOT`
environment variable to manipulate the root of all Habitat paths.

This change preserves the strategy of these tests but rather introduces
a test-only environment variable which the test suite can set, knowing
what the consequences will be (see above). Additionally, if this
environment variable is used, a message will be printed to the standard
error stream notifying any user that this is intended only for testing
and not production. Note that by default, the test suites have their
stdout/stderr streams redirected and so this warning wouldn't appear
unless the `--nocapture` flag is added. This is one instance where the
rule above ("don't honor environment varibles in inner library code")
isn't an unbreakable law, it just needs to be carefully considered.

Signed-off-by: Fletcher Nichol <fnichol@nichol.ca>
@fnichol fnichol force-pushed the fnichol/explicit-testing-fs-root branch from 90503bd to 68013fa Compare August 29, 2017 22:29
@fnichol
Copy link
Collaborator Author

fnichol commented Aug 29, 2017

And now with the Travis fix

tenor-80078196

@fnichol
Copy link
Collaborator Author

fnichol commented Aug 29, 2017

@thesentinels approve

@thesentinels
Copy link
Contributor

🤘 I am testing your branch against master before merging it. We do this to ensure that the master branch is never failing tests.

@thesentinels
Copy link
Contributor

:neckbeard: Travis CI has started testing this PR.

@thesentinels
Copy link
Contributor

💖 Travis CI reports this PR passed.

It always makes me feel nice when humans approve of one anothers work. I'm merging this PR now.

I just want you and the contributor to answer me one question:

gif-keyboard-3280869874741411265

@thesentinels thesentinels merged commit 216fd27 into master Aug 29, 2017
@thesentinels thesentinels deleted the fnichol/explicit-testing-fs-root branch August 29, 2017 23:25
@eeyun eeyun added I-linux and removed P-linux labels Mar 6, 2018
@eeyun eeyun added I-windows and removed P-macOS labels Mar 6, 2018
@christophermaier christophermaier added Focus:Supervisor Related to the Habitat Supervisor (core/hab-sup) component Focus: CLI Related to the Habitat CLI (core/hab) component Platform: Linux Deals with Linux-specific behavior Platform: Windows Deals with Windows-specific behavior Platform: macOS Deals with macOS-specific behavior Type: Bug Issues that describe broken functionality and removed A-supervisor labels Jul 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Focus: CLI Related to the Habitat CLI (core/hab) component Focus:Supervisor Related to the Habitat Supervisor (core/hab-sup) component Platform: Linux Deals with Linux-specific behavior Platform: macOS Deals with macOS-specific behavior Platform: Windows Deals with Windows-specific behavior Type: Bug Issues that describe broken functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants