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

Ensure studio is installed #6772

Merged
merged 3 commits into from Aug 5, 2019

Conversation

@smacfarlane
Copy link
Contributor

commented Jul 30, 2019

Closes #6771

Previously, studio enter would ensure that the appropriate core/hab-studio package was installed before attempting to invoke it. With the change in #6549, we attempt to read the metadata from the studio package, assuming it is already installed which isn't the case after installing a new release of Habitat.

This corrects that by ensuring the studio package is installed before attempting to read the metadata.

Signed-off-by: Scott Macfarlane smacfarlane@chef.io

Signed-off-by: Scott Macfarlane <smacfarlane@chef.io>
@@ -114,6 +114,7 @@ mod inner {
let ident = PackageIdent::from_str(&format!("{}/{}",
super::STUDIO_PACKAGE_IDENT,
version[0]))?;
let command = exec::command_from_min_pkg(ui, super::STUDIO_CMD, &ident)?

This comment has been minimized.

Copy link
@smacfarlane

smacfarlane Jul 30, 2019

Author Contributor

This continues the trend in this code path of "spooky action at a distance". This not only gets the command to install (which returns a PathBuf rather than a Cmd), but it also manages the installation of the studio package. It also now occurs early in the function so that we can extract the metadata before returning the command value.

This should be the minimum change necessary to fix the underlying issue (still testing to verify), but depending on the timing of shipping this fix in a release I'd like to refactor this to clean up the responsibilities of these functions to make it clear how/when a studio package is installed.

smacfarlane added 2 commits Jul 30, 2019
Signed-off-by: Scott Macfarlane <smacfarlane@chef.io>
Signed-off-by: Scott Macfarlane <smacfarlane@chef.io>
@@ -114,6 +114,7 @@ mod inner {
let ident = PackageIdent::from_str(&format!("{}/{}",
super::STUDIO_PACKAGE_IDENT,
version[0]))?;
let command = exec::command_from_min_pkg(ui, super::STUDIO_CMD, &ident)?;
// This is a duplicate of the code in `hab pkg exec` and
// should be refactored as part of or after:
// https://github.com/habitat-sh/habitat/issues/6633

This comment has been minimized.

Copy link
@christophermaier

christophermaier Jul 30, 2019

Contributor

Is it maybe time for this refactoring?

This comment has been minimized.

Copy link
@smacfarlane

smacfarlane Jul 30, 2019

Author Contributor

Yes 😹 I started down this path a couple weeks ago, but had to set it aside due to time constraints. I've updated my notes on the tracking issue #6634

In addition to the above refactor, I think we'd possibly want to pull the "ensure package is installed" code out of command_from_min_pkg so that we can isolate the responsibilities of that code. That function appears to only be used in a few places through the code base, but it is used by the launcher and supervisor, in addition to studio enter.

This comment has been minimized.

Copy link
@baumanj

baumanj Aug 1, 2019

Contributor

I think we'd possibly want to pull the "ensure package is installed" code out of command_from_min_pkg

Strong agree.

Replacing the PackageInstall::load on L121 with something like ::install_and_load makes this more intuitive, I think. In fact, how often do we really want PackageInstall::load to fail if the package isn't installed versus trying the install? At a quick glance, it appears to be nearly always called with ?. This might be the time to do a little audit of the callers and see if we have more latent bugs which are similar to this.

This comment has been minimized.

Copy link
@smacfarlane

smacfarlane Aug 1, 2019

Author Contributor

I was thinking, based on how PackageIdent and PackageInstall are used here, that it would make sense to have method on PackageIdent that returns a PackageInstall and handles the installation if needed.

Perhaps there's a more Rusty way to handle it, but given that PackageInstall::load takes a PackageIdent, we probably have this pattern throughout the code and it, maybe naively, would make sense to have that behavior baked into the transformation of ident into install.

This comment has been minimized.

Copy link
@baumanj

baumanj Aug 1, 2019

Contributor

If what we have is a PackageIdent, and want is a PackageInstall, the most rustic approaches to me seem to be adding PackageInstall::from_ident() -> Self or PackageIdent::install(&self) -> PackageInstall. I like the way the latter reads, but since PackageInstall has non-pub fields, you'll probably need to call on PackageInstall to create it for you. In that case implementing PackageIdent::install by having it call PackageInstall::from_ident might be appropriate.

But now I see that we also have an InstallTask type, which already has a with_ident method that returns a Result<PackageInstall>, so perhaps that's what we should be using. It's currently private, but it probably makes sense to expose at least part of that interface so we have a cleaner way to go from PackageIdentPackageInstall. The complication is that a number of other inputs are necessary.

Copy link
Contributor

left a comment

Looks good so far, and seems more correct and explicit than what was there before.

Haven't had a chance to think through broader implications (if any), but it seems like a good start.

@scotthain

This comment has been minimized.

Copy link
Contributor

commented Jul 31, 2019

This will resolve #6775

Copy link
Contributor

left a comment

This does appear to do fix the direct issue; so I wouldn't oppose this going in as is in the short term.

However, I agree with the sentiments to split up command_from_min_pkg so we can have "install (if needed) and then load" functionality. We should also add unit tests and audit the other callers of PackageInstall::load to check for similar defects. This should happen as a direct follow-up so we don't lose the context.

Let us know how you'd like to proceed, and then move this out of draft mode when you'd like formal approval.

@@ -114,6 +114,7 @@ mod inner {
let ident = PackageIdent::from_str(&format!("{}/{}",
super::STUDIO_PACKAGE_IDENT,
version[0]))?;
let command = exec::command_from_min_pkg(ui, super::STUDIO_CMD, &ident)?;
// This is a duplicate of the code in `hab pkg exec` and
// should be refactored as part of or after:
// https://github.com/habitat-sh/habitat/issues/6633

This comment has been minimized.

Copy link
@baumanj

baumanj Aug 1, 2019

Contributor

I think we'd possibly want to pull the "ensure package is installed" code out of command_from_min_pkg

Strong agree.

Replacing the PackageInstall::load on L121 with something like ::install_and_load makes this more intuitive, I think. In fact, how often do we really want PackageInstall::load to fail if the package isn't installed versus trying the install? At a quick glance, it appears to be nearly always called with ?. This might be the time to do a little audit of the callers and see if we have more latent bugs which are similar to this.

@smacfarlane smacfarlane marked this pull request as ready for review Aug 1, 2019
@smacfarlane

This comment has been minimized.

Copy link
Contributor Author

commented Aug 1, 2019

@baumanj Let's go with this as-is so at minimum we have a fix in the next release. I can swing back to this once the core-plans refresh is complete, or someone else can pick this work up in the mean time.

@baumanj
baumanj approved these changes Aug 1, 2019
@smacfarlane smacfarlane merged commit e38dbdd into master Aug 5, 2019
5 checks passed
5 checks passed
DCO This commit has a DCO Signed-off-by
Details
buildkite/habitat-sh-habitat-master-verify Build #2862 passed (19 minutes, 29 seconds)
Details
buildkite/habitat-sh-habitat-master-website Build #3687 passed (35 seconds)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
expeditor/config-validation Validated your Expeditor config file
Details
@chef-ci chef-ci deleted the sm/fix-studio-enter branch Aug 5, 2019
@mwrock mwrock added the X-fix label Aug 21, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.