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

Wire hab-pkg-export-docker into hab & setup for building. #3334

Merged
merged 3 commits into from Sep 26, 2017
Merged

Wire hab-pkg-export-docker into hab & setup for building. #3334

merged 3 commits into from Sep 26, 2017

Conversation

fnichol
Copy link
Collaborator

@fnichol fnichol commented Sep 26, 2017

There are a few changes in this PR, split out to make things a bit clearer.

  • The removal of old functional testing setup was done when searching the source tree for references to dockerize.
  • A hab-only change to update the CLI delegating code to invoke the new Rust program directly and avoid the existing common hab pkg export logic.
  • Add the package into the build and release process, driven primarily by .travis.yml.

@fnichol
[builder-depot] Remove unused function setup code.
630638e 
This is a follow up to #3146 which removed functional tests (in
c83df15) which were mocked. If this
strategy or of this code becomes viable again, it can be resurrected via
Git.

Signed-off-by: Fletcher Nichol <fnichol@nichol.ca>
@fnichol
[hab] Directly invoke updated docker exporter via exec.
47dfaf5 
This change performs the same install-on-use logic as `hab sup` and `hab
studio` subcommands. The underlying program, `hab-pkg-export-docker`
will be directly invoked and therefore:

```
hab pkg export docker --help
```

Will return the usage from `hab-pkg-export-docker` in the
`core/hab-pkg-export-docker` package

The `exec_subcommand_if_called()` function in the main module was
widened to look at 3 subcommands deep, rather the 2 subcommands prior
to this change. This was necessary as due to the subcommand depth of
`hab pkg export docker`. Future exporters should not need to do this.

Signed-off-by: Fletcher Nichol <fnichol@nichol.ca>
@fnichol
[.travis] Add pkg-export-docker into release process & drop dockerize.
d8b88af 
This change swaps out the building of `pkg-dockerize` for
`pkg-export-docker`, meaning that unstable builds and releases will
start building the new component.

Signed-off-by: Fletcher Nichol <fnichol@nichol.ca>
@fnichol fnichol added this to the Pre-Neighborhoods milestone Sep 26, 2017
@fnichol fnichol self-assigned this Sep 26, 2017
@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
Copy link
Collaborator Author

fnichol commented Sep 26, 2017

I'm going to take a quick swing at compiling on macOS to confirm the Mac behavior (warn and quit).

reset
reset reviewed Sep 26, 2017
View reviewed changes
components/hab/src/command/pkg/export/docker.rs
let command = match henv::var(EXPORT_CMD_ENVVAR) {
Ok(command) => PathBuf::from(command),
Err(_) => {
init();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we be starting this earlier in the function? I think I made changes recently so this wouldn't panic if you called it more than once - can't remember exactly

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We could do a future refactor which just jams that early in hab. We need this for the install logic/verify code and have been only calling it when necessary. But yeah, I've never been 100% happy with that. It's possible that the overhead of that call is so small that it isn't an issue.

@reset reset self-requested a review September 26, 2017 03:18
reset
reset approved these changes Sep 26, 2017
View reviewed changes
Copy link
Collaborator

@reset reset left a comment

Choose a reason for hiding this comment

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

Merge when you've done your testing, it looks like a clean replacement to me!

@fnichol
Copy link
Collaborator Author

fnichol commented Sep 26, 2017

I should note that for the moment, I left components/pkg-dockerize in the source tree as I'm not 100% sure if it's being depended upon by any other software. We should be good to remove that in a few weeks unless we learn anything.

@fnichol
Copy link
Collaborator Author

fnichol commented Sep 26, 2017

Took me a couple of tries on my rebuilt macOS to make sure I had all linked library deps. Looks good though.

tenor-97992166

@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 8f32684 into master Sep 26, 2017
@thesentinels thesentinels deleted the fnichol/ship-rs-docker-exporter branch September 26, 2017 04:05
@eeyun eeyun added I-linux and removed P-linux labels Mar 6, 2018
@christophermaier christophermaier added 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: Feature Issues that describe a new desired feature and removed A-cli labels Jul 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@reset reset reset approved these changes

@chefsalim chefsalim Awaiting requested review from chefsalim

@cnunciato cnunciato Awaiting requested review from cnunciato

@mgamini mgamini Awaiting requested review from mgamini

@raskchanky raskchanky Awaiting requested review from raskchanky

Assignees

@fnichol fnichol

Labels
Focus: CLI Related to the Habitat CLI (core/hab) component Focus:Exporter Platform: Linux Deals with Linux-specific behavior Platform: macOS Deals with macOS-specific behavior Platform: Windows Deals with Windows-specific behavior Type: Feature Issues that describe a new desired feature
Projects
None yet
Milestone
Pre-Neighborhoods
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@fnichol @thesentinels @reset @christophermaier @eeyun