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.

Sign up for GitHub

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

Jump to bottom

Corrects Binlink in Studio, adds HAB_BINLINK_DIR envvar #2951

Merged
merged 1 commit into from Aug 16, 2017
Merged

Corrects Binlink in Studio, adds HAB_BINLINK_DIR envvar #2951

merged 1 commit into from Aug 16, 2017

Conversation

eeyun
Copy link
Contributor

@eeyun eeyun commented Aug 16, 2017

Signed-off-by: Ian Henry ihenry@chef.io

@eeyun eeyun requested a review from reset as a code owner August 16, 2017 15:35
@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!

@eeyun eeyun mentioned this pull request Aug 16, 2017
Closed
@eeyun eeyun requested a review from fnichol August 16, 2017 15:40
@eeyun eeyun self-assigned this Aug 16, 2017
@eeyun
Copy link
Contributor Author

eeyun commented Aug 16, 2017

Fixes #975

@eeyun eeyun added the A-studio label Aug 16, 2017
@ShalokShalom
Copy link

Can you tell me what is the default value, so if this environment variable is not set?

@eeyun
Copy link
Contributor Author

eeyun commented Aug 16, 2017

@ShalokShalom the default location doesnt change with this PR. Basically https://github.com/habitat-sh/habitat/pull/2951/files#diff-aa992c32664dad8d8e1a241ef1d50bcfR18 we default to /bin like we were before. This change just also provides HAB_BINLINK_DIR envvar that we will export by default in the studio to a directory that is on the path by default https://github.com/habitat-sh/habitat/pull/2951/files#diff-98ec64e3f381a5ccde4f4506db6aae9cR98.

@eeyun
Copy link
Contributor Author

eeyun commented Aug 16, 2017

Its a bit more flexible and safe way to implement the other PR.

mwrock
mwrock reviewed Aug 16, 2017
View reviewed changes
components/hab/src/main.rs Outdated
@@ -287,7 +287,8 @@ fn sub_origin_key_upload(ui: &mut UI, m: &ArgMatches) -> Result<()> {

fn sub_pkg_binlink(ui: &mut UI, m: &ArgMatches) -> Result<()> {
let ident = PackageIdent::from_str(m.value_of("PKG_IDENT").unwrap())?;
let dest_dir = Path::new(m.value_of("DEST_DIR").unwrap_or(DEFAULT_BINLINK_DIR));
let env_or_default = henv::var(BINLINK_DIR_ENVVAR).unwrap_or(DEFAULT_BINLINK_DIR.to_string());
Copy link
Contributor

Choose a reason for hiding this comment

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

Should you call the function you created above? default_binlink_dir which appears to be unused right now.

Copy link
Contributor Author

@eeyun eeyun Aug 16, 2017 •
edited

Choose a reason for hiding this comment

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

We definitely can. I added that in case it was useful elsewhere. The syntax here was just to match the way we're handling other ENVVAR stuff for the depot bits within the same script

Copy link
Contributor

Choose a reason for hiding this comment

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

since its basically doing the same thing as that function, I'd either use it or drop the function

mwrock
mwrock reviewed Aug 16, 2017
View reviewed changes
components/hab/src/main.rs Outdated
@@ -454,7 +455,9 @@ fn sub_pkg_install(ui: &mut UI, m: &ArgMatches) -> Result<()> {
ignore_target,
)?;
if m.is_present("BINLINK") {
let dest_dir = Path::new(m.value_of("DEST_DIR").unwrap_or(DEFAULT_BINLINK_DIR));
let env_or_default =
henv::var(BINLINK_DIR_ENVVAR).unwrap_or(DEFAULT_BINLINK_DIR.to_string());
Copy link
Contributor

Choose a reason for hiding this comment

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

same comment as above

Corrected Binlinking in Studio, adds HAB_BINLINK_DIR envvar
7d68c4e 
Signed-off-by: Ian Henry <ihenry@chef.io>
@reset
Copy link
Collaborator

reset commented Aug 16, 2017

@thesentinels approve

tenor-64974529

@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.

@fnichol
Copy link
Collaborator

fnichol commented Aug 16, 2017

Looks great!

tenor-125766983

@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 28f5019 into master Aug 16, 2017
@thesentinels thesentinels deleted the eeyun/binlink_fixup branch August 16, 2017 18:23
@reset reset added this to In Acceptance in Core Engineering Aug 16, 2017
@reset reset mentioned this pull request Aug 16, 2017
Closed
@tashimi tashimi moved this from In Acceptance to Accepted in Core Engineering Aug 21, 2017
@tashimi tashimi removed this from Accepted in Core Engineering Aug 22, 2017
@fnichol fnichol mentioned this pull request Dec 23, 2017
Closed
@christophermaier christophermaier added Focus: Studio Related to the Habitat Studio (core/hab-studio) component Focus: CLI Related to the Habitat CLI (core/hab) component Type: Chore Issues for general code and infrastructure maintenance and removed A-studio labels Jul 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mwrock mwrock mwrock left review comments

@reset reset reset approved these changes

@fnichol fnichol Awaiting requested review from fnichol

Assignees

@eeyun eeyun

Labels
Focus: CLI Related to the Habitat CLI (core/hab) component Focus: Studio Related to the Habitat Studio (core/hab-studio) component Type: Chore Issues for general code and infrastructure maintenance
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@eeyun @thesentinels @ShalokShalom @reset @fnichol @mwrock @christophermaier