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

Check for missing cache key path when entering dockerized studio #6345

Merged
merged 4 commits into from Jun 13, 2019

Conversation

@danielhertenstein
Copy link
Contributor

commented Mar 26, 2019

Resolves #5025

I ran into issue #5025 when setting up Habitat on my Windows machine, so I figured I would submit a pull request along the lines of the solution suggested by @mwrock.

Error::FileNotFound() felt the closest to this error, but I am happy to switch it to something else or add a new enum element.

I know issue #6314 also exists which intends to touch this code, but from what I can tell, a check would still be good here even if the way the path arrives in this method changes.

Lastly, I had started writing a test for this, but noticed the verification pipeline does not run any tests for studio and only checks to ensure it builds. It looks like I'd have to change the pipeline in order for any tests I write to be run, and I don't know what you all think about that.

Edit: I forgot to note that I needed to make the change to build_component.ps1 in order to run the build test locally. The comment above the pkg build line made it seem like this change should have been made a little bit ago.

@thesentinels

This comment has been minimized.

Copy link
Contributor

commented Mar 26, 2019

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!

@baumanj

This comment has been minimized.

Copy link
Member

commented Mar 26, 2019

Thanks for the PR!

I have a change in flight as part of #6314 that will add a cache_key_path: &Path argument to start_docker_studio. I believe that will supersede this change since we're trying to get away from using default_cache_key_path altogether. If you see any reason that's not the case, let's discuss, otherwise I apologize for any wasted effort on your part, I've only just recently started working on this.

@baumanj

This comment has been minimized.

Copy link
Member

commented Mar 26, 2019

After looking a bit more, I think we may want to do something like what you have here, but just at a different level. Let me get a PR up for what I'm doing and we can re-assess.

@danielhertenstein

This comment has been minimized.

Copy link
Contributor Author

commented Mar 26, 2019

I saw you've been tackling #6314, and was also thinking this sort of check would wind up being moved to a higher level after this section of code was addressed, so that sounds good from my perspective.

@mwrock mwrock removed their assignment Apr 4, 2019

@baumanj

This comment has been minimized.

Copy link
Member

commented Apr 9, 2019

I haven’t forgotten about this, it’s just gotten a bit delayed behind some other work. Will update when I’m able to finish up 6314

@danielhertenstein

This comment has been minimized.

Copy link
Contributor Author

commented Apr 19, 2019

No worries. I understand having a lot of work to do at the same time. I'm happy to look back into this whenever #6314 is done.

@baumanj

This comment has been minimized.

Copy link
Member

commented Apr 29, 2019

Just got back from a week out, so hopefully will have a meaningful update here by sometime next week.

@danielhertenstein

This comment has been minimized.

Copy link
Contributor Author

commented May 1, 2019

Sounds good. Welcome back, and good luck.

@baumanj
Copy link
Member

left a comment

Hey @danielhertenstein. Thanks again for this contribution and your patience in waiting for feedback.

It looks like #6314 isn't going to be the top priority at the moment, so let's move forward here. I have some small points of feedback, but mostly this just needs a rebase and should be good to go.

components/hab/src/command/studio/docker.rs Outdated Show resolved Hide resolved
components/hab/src/command/studio/docker.rs Outdated Show resolved Hide resolved
components/hab/src/command/studio/docker.rs Outdated Show resolved Hide resolved
support/ci/build_component.ps1 Outdated Show resolved Hide resolved
@danielhertenstein

This comment has been minimized.

Copy link
Contributor Author

commented May 21, 2019

@baumanj Thanks for looking back into this. I'll have plenty of time to fix this up next week, if that's an okay wait for you.

@baumanj

This comment has been minimized.

Copy link
Member

commented Jun 10, 2019

Hey @danielhertenstein. Is this still something you're interested in continuing with?

@danielhertenstein

This comment has been minimized.

Copy link
Contributor Author

commented Jun 11, 2019

Yes, still interested. Sorry for the delay.

@baumanj

This comment has been minimized.

Copy link
Member

commented Jun 11, 2019

No worries; just wanted to check.

@danielhertenstein danielhertenstein force-pushed the danielhertenstein:missing-cache-key-path branch from 8e4a062 to 3546bc4 Jun 12, 2019

Checks if default cache key path exists and tells user if it doesn't.
Signed-off-by: Daniel Hertenstein <daniel.hertenstein@gmail.com>

@danielhertenstein danielhertenstein force-pushed the danielhertenstein:missing-cache-key-path branch from 3546bc4 to 160528f Jun 12, 2019

danielhertenstein added some commits Jun 12, 2019

Removes call to default_cache_key_path()
Signed-off-by: Daniel Hertenstein <daniel.hertenstein@gmail.com>
Changes `.to_string_lossy()` calls to `.display()` calls
Signed-off-by: Daniel Hertenstein <daniel.hertenstein@gmail.com>
@danielhertenstein

This comment has been minimized.

Copy link
Contributor Author

commented Jun 13, 2019

@baumanj, I've committed the requested changes, though I'm not sure how to move the pull request forward via the interface here. Do I hit "resolve conversation" on everything?

@baumanj baumanj referenced this pull request Jun 13, 2019
@baumanj
Copy link
Member

left a comment

This needs a small tweak for rustfmt. See https://buildkite.com/chef/habitat-sh-habitat-master-verify/builds/2345#89270066-0aa8-4bc7-afa6-040e80c9bbfa.

We use a nightly rust version for rustfmt (documented here), so let me know if you run into any issues there. Once you have that installed, it should be as simple as cargo +nightly fmt at the root.

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.

rustfmt changes
Signed-off-by: Daniel Hertenstein <daniel.hertenstein@gmail.com>
@danielhertenstein

This comment has been minimized.

Copy link
Contributor Author

commented Jun 13, 2019

@baumanj, neither of those links worked for me. The first one required I log into Okta. The second one 404'd. But the formatting is done :)

@baumanj baumanj merged commit 88cb993 into habitat-sh:master Jun 13, 2019

4 of 5 checks passed

buildkite/habitat-sh-habitat-master-verify Build #2362 failed (23 minutes, 52 seconds)
Details
DCO This commit has a DCO Signed-off-by
Details
buildkite/habitat-sh-habitat-master-website Build #2734 passed (31 seconds)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
expeditor/config-validation Validated your Expeditor config file
Details

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

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

This comment has been minimized.

Copy link
Member

commented Jun 13, 2019

Thanks so much for the contribution, @danielhertenstein !

@danielhertenstein

This comment has been minimized.

Copy link
Contributor Author

commented Jun 13, 2019

@baumanj, I was happy to do so. Thanks for helping me through it!

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