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

Load config variables from cli.toml when entering hab-studio #6644

Merged
merged 14 commits into from Jun 14, 2019

Conversation

@qubitrenegade
Copy link
Contributor

commented Jun 11, 2019

This commit loads from cli.toml and sets:

  • HAB_AUTH_TOKEN
  • HAB_BLDR_URL
  • HAB_CTL_SECRET

If these variables are not set before hab studio enter is executed.

This resolves #4908

@chef-expeditor

This comment has been minimized.

Copy link

commented Jun 11, 2019

Hello qubitrenegade! 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!

@qubitrenegade

This comment has been minimized.

Copy link
Contributor Author

commented Jun 11, 2019

Ok, how do I test this? I for the life of me can't get into the hab studio... however, I do see:

ubuntu@ubuntu:~/habitat$ RUST_LOG=debug cargo run -p hab studio enter 2>&1 | grep hab::command::studio::enter
[2019-06-11T02:51:15Z DEBUG hab::command::studio::enter] Setting HAB_AUTH_TOKEN=FOOBARBIZBAZ via config file
[2019-06-11T02:51:15Z DEBUG hab::command::studio::enter] Setting HAB_BLDR_URL=https://foo.bar.com via config file
[2019-06-11T02:51:15Z DEBUG hab::command::studio::enter] Setting HAB_CTL_SECRET=BAZBOZBOOZE via config file
[2019-06-11T02:51:15Z DEBUG hab::command::studio::enter] Setting default origin originfoo via CLI config
[2019-06-11T02:51:15Z DEBUG hab::command::studio::enter] Setting HAB_CACHE_KEY_PATH=/home/ubuntu/.hab/cache/keys
[2019-06-11T02:51:15Z DEBUG hab::command::studio::enter] Setting ARTIFACT_PATH=/home/ubuntu/.hab/cache/artifacts

When I try to set the HAB_STUDIO_BINARY, it fails being unable to find /etc/passwd but there is one in components/studio/defaults/etc/passwd and my user has permissions to read the system /etc/passwd, so I don't understand the error...

ubuntu@ubuntu:~/habitat$ env HAB_STUDIO_BINARY=/home/ubuntu/habitat/components/studio/bin/hab-studio.sh target/debug/hab studio enter
   hab-studio.sh: Creating Studio at /hab/studios/home--ubuntu--habitat (default)
ERROR: Tried to copy default file for '/etc/passwd', but could not find one! Please report this error.
ubuntu@ubuntu:~/habitat$ target/debug/hab studio enter
∵ Missing package for core/hab-studio/0.83.0-dev
» Installing core/hab-studio/0.83.0-dev
☁ Determining latest version of core/hab-studio/0.83.0-dev in the 'stable' channel
✗✗✗
✗✗✗ Package not found. 
✗✗✗
ubuntu@ubuntu:~/habitat$ HAB_STUDIO_BINARY=/home/ubuntu/habitat/components/studio/bin/hab-studio.sh cargo run -p hab studio enter
    Finished dev [unoptimized + debuginfo] target(s) in 0.27s
     Running `target/debug/hab studio enter`
   hab-studio.sh: Creating Studio at /hab/studios/home--ubuntu--habitat (default)
ERROR: Tried to copy default file for '/etc/passwd', but could not find one! Please report this error.
@baumanj

This comment has been minimized.

Copy link
Member

commented Jun 11, 2019

Ok, how do I test this?

Have you taken a look at https://github.com/habitat-sh/habitat/blob/master/BUILDING.md#testing-changes ?

@baumanj
Copy link
Member

left a comment

There was already a bit of duplication here, but with the additions of these 3 items, I think it's worth abstracting it out. If we add something like:

fn set_env_var_from_config(env_var: &str, config_val: Option<String>) {
    if henv::var(env_var).is_err() {
        if let Some(val) = config_val {
            debug!("Setting {}={} via config file", env_var, val);
            env::set_var(env_var, val);
        }
    }
}

we could simplify several of them to

    let config = config::load()?;

    set_env_var_from_config(AUTH_TOKEN_ENVVAR, config.auth_token);
    set_env_var_from_config(BLDR_URL_ENVVAR, config.bldr_url);
    …

But artifact_path breaks that pattern. We could change set_env_var_from_config to return an Option<String> and make a generalization that could accommodate artifact_path as well, but I'll leave it to you whether you want to go down that road or not.

components/hab/src/command/studio/enter.rs Outdated Show resolved Hide resolved
components/hab/src/command/studio/enter.rs Outdated Show resolved Hide resolved
components/hab/src/command/studio/enter.rs Outdated Show resolved Hide resolved
components/hab/src/command/studio/enter.rs Outdated Show resolved Hide resolved
components/hab/src/command/studio/enter.rs Outdated Show resolved Hide resolved
components/hab/src/command/studio/enter.rs Outdated Show resolved Hide resolved
components/hab/src/command/studio/enter.rs Outdated Show resolved Hide resolved
@qubitrenegade

This comment has been minimized.

Copy link
Contributor Author

commented Jun 11, 2019

Ok, how do I test this?

Have you taken a look at https://github.com/habitat-sh/habitat/blob/master/BUILDING.md#testing-changes ?

Yes :)

I set the HAB_STUDIO_BINARY, but then it errors that it can't find /etc/passwd.

ubuntu@ubuntu:~/habitat$ HAB_STUDIO_BINARY=/home/ubuntu/habitat/components/studio/bin/hab-studio.sh cargo run -p hab studio enter
    Finished dev [unoptimized + debuginfo] target(s) in 0.27s
     Running `target/debug/hab studio enter`
   hab-studio.sh: Creating Studio at /hab/studios/home--ubuntu--habitat (default)
ERROR: Tried to copy default file for '/etc/passwd', but could not find one! Please report this error.
@qubitrenegade

This comment has been minimized.

Copy link
Contributor Author

commented Jun 11, 2019

There was already a bit of duplication here, but with the additions of these 3 items, I think it's worth abstracting it out. If we add something like:

fn set_env_var_from_config(env_var: &str, config_val: Option<String>) {
    if henv::var(env_var).is_err() {
        if let Some(val) = config_val {
            debug!("Setting {}={} via config file", env_var, val);
            env::set_var(env_var, val);
        }
    }
}

we could simplify several of them to

    let config = config::load()?;

    set_env_var_from_config(AUTH_TOKEN_ENVVAR, config.auth_token);
    set_env_var_from_config(BLDR_URL_ENVVAR, config.bldr_url);
    …

But artifact_path breaks that pattern. We could change set_env_var_from_config to return an Option<String> and make a generalization that could accommodate artifact_path as well, but I'll leave it to you whether you want to go down that road or not.

Ah ha! I was still thinking we might config::load(); in set_env_var_from_config().

Your name is a bit more verbose than mine ("set_env_var"), but is more explicit/obvious so I used that :)

There's a comment somewhere, but what is config.auth_token? Why is in an Option<String>?

I was totally not expecting a reply so late!

@baumanj

This comment has been minimized.

Copy link
Member

commented Jun 11, 2019

As for testing, I didn't look closely enough at what you're doing. There's no need to use the HAB_STUDIO_BINARY override, since you're not modifying hab-studio.sh. You should be able to test your changes with just

cargo run -p hab studio enter
@baumanj

This comment has been minimized.

Copy link
Member

commented Jun 11, 2019

There's a comment somewhere, but what is config.auth_token? Why is in an Option<String>?

It's the value of auth_token as read from cli.toml. It's an Option because it may or may not be present.

@qubitrenegade

This comment has been minimized.

Copy link
Contributor Author

commented Jun 12, 2019

cargo run -p hab studio enter

That bombs out unable to find core/hab-studio/0.83.0-dev

ubuntu@ubuntu:~/habitat$ cargo run -p hab studio enter 2>&1
    Finished dev [unoptimized + debuginfo] target(s) in 0.27s
     Running `target/debug/hab studio enter`
[sudo hab-studio] password for ubuntu: 
∵ Missing package for core/hab-studio/0.83.0-dev
» Installing core/hab-studio/0.83.0-dev
☁ Determining latest version of core/hab-studio/0.83.0-dev in the 'stable' channel
✗✗✗
✗✗✗ failed to lookup address information: Name or service not known
✗✗✗

qubitrenegade added some commits Jun 7, 2019

add an env var... Build env is broken on FC29 so moving to ubuntu vm …
…for testing

Signed-off-by: qubitrenegade <qubitrenegade@gmail.com>
fix typos
Signed-off-by: qubitrenagade <qubitrenegade@gmail.com>
Add auth_token, bldr_url, and ctl_secret to variables loaded when ent…
…ering hab studio

Signed-off-by: qubitrenagade <qubitrenegade@gmail.com>
create set_env_var function to reduce some of the copy/pasting
Signed-off-by: qubitrenagade <qubitrenegade@gmail.com>
Further abstract out set_env_var_from_config to reduce copy/pasted co…
…de and make code more obvious

Signed-off-by: qubitrenagade <qubitrenegade@gmail.com>
rustfmt cleanup: `cargo +nightly-2019-03-04 fmt
Signed-off-by: qubitrenagade <qubitrenegade@gmail.com>
Don't print sensitive values with RUST_LOG=debug
Signed-off-by: qubitrenagade <qubitrenegade@gmail.com>
Use bool instead of Option<bool>
Signed-off-by: qubitrenagade <qubitrenegade@gmail.com>
Import consts from where they are defined instead of defining locally
Signed-off-by: qubitrenagade <qubitrenegade@gmail.com>
Cleanup dead code
Signed-off-by: qubitrenagade <qubitrenegade@gmail.com>
rustfmt cleanup
Signed-off-by: qubitrenagade <qubitrenegade@gmail.com>

@qubitrenegade qubitrenegade force-pushed the qubitrenegade:qbrd/load-env-vars branch from 1cf7f03 to 9d113a6 Jun 12, 2019

@baumanj

This comment has been minimized.

Copy link
Member

commented Jun 13, 2019

```shell
cargo run -p hab studio enter

That bombs out unable to find core/hab-studio/0.83.0-dev

That's ok though because it's after your code runs, right? I mean, it's not perfect, but it should be enough to run your code.

You might be able to get further with setting HAB_STUDIO_BINARY, but I think looking at the debug output is enough to validate your code is working.

Add Sensitivity enum to ensure sensitive information is not printed
Signed-off-by: qubitrenagade <qubitrenegade@gmail.com>
@qubitrenegade

This comment has been minimized.

Copy link
Contributor Author

commented Jun 13, 2019

```shell
cargo run -p hab studio enter

That bombs out unable to find core/hab-studio/0.83.0-dev

That's ok though because it's after your code runs, right? I mean, it's not perfect, but it should be enough to run your code.

You might be able to get further with setting HAB_STUDIO_BINARY, but I think looking at the debug output is enough to validate your code is working.

Correct, this runs before the "sudo prompt" to actually enter the studio.

This code is being executed so assuming env::set_var does the right thing, I'm like... 75%...? certain this does what we want/expect and will leave the studio in the desired state.

rustfmt
Signed-off-by: qubitrenagade <qubitrenegade@gmail.com>
@baumanj baumanj referenced this pull request Jun 13, 2019
@baumanj
Copy link
Member

left a comment

This just needs a small tweak for a clippy issue in our CI.

Currently our CI can't be triggered by external contributors (we're working on a fix), so I had to set up a clone of your branch to run it. Don't worry about any of the other issues you may see in that CI run, only the one clippy thing is related to your code.

Add traits to Enum
Add Clone and Copy traits to `Sensitivity` enum to fix "needless pass by value" clippy gripe.

Co-Authored-By: baumanj <baumanj@users.noreply.github.com>

@baumanj baumanj merged commit ff39e02 into habitat-sh:master Jun 14, 2019

3 of 5 checks passed

DCO This commit does not have a DCO Signed-off-by
Details
buildkite/habitat-sh-habitat-master-verify Build #2374 failed (1 hour, 24 minutes, 44 seconds)
Details
buildkite/habitat-sh-habitat-master-website Build #2756 passed (1 minute, 6 seconds)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
expeditor/config-validation Validated your Expeditor config file
Details
@baumanj

This comment has been minimized.

Copy link
Member

commented Jun 14, 2019

Thanks again for a great submission, @qubitrenegade!

chef-ci added a commit that referenced this pull request Jun 14, 2019

Update CHANGELOG.md with details from pull request #6644
Obvious fix; these changes are the result of automation not creative thinking.
@bdangit

This comment has been minimized.

Copy link
Contributor

commented Jun 15, 2019

Agree! This is awesome!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.