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

Complete pack-interact workflow #1278

Merged
merged 2 commits into from
Nov 12, 2021

Conversation

aemengo
Copy link
Contributor

@aemengo aemengo commented Sep 7, 2021

Summary

Add remaining functionality for pack-interact workflow

This PR kicks continues the following user workflow:

When a user runs a pack build like so: pack build <image-name> --interactive
Then the user will view the build process through a terminal UI screen
First the Detecting... page will show
Then the build "dashboard" page will show

Documentation

  • Should this change be documented?
    • Yes, see #___
    • No

Related

Resolves:
#1203
#1204
#1205
#1206

@github-actions github-actions bot added the type/enhancement Issue that requests a new feature or improvement. label Sep 7, 2021
@github-actions github-actions bot added this to the 0.22.0 milestone Sep 7, 2021
@aemengo aemengo force-pushed the pack-interact-mode-continued branch 3 times, most recently from 50e1efc to 229092a Compare September 13, 2021 20:17
@codecov
Copy link

codecov bot commented Sep 16, 2021

Codecov Report

Merging #1278 (eede4d2) into main (339b49e) will increase coverage by 0.22%.
The diff coverage is 90.35%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1278      +/-   ##
==========================================
+ Coverage   80.65%   80.86%   +0.22%     
==========================================
  Files         143      144       +1     
  Lines        8998     9152     +154     
==========================================
+ Hits         7256     7400     +144     
- Misses       1275     1283       +8     
- Partials      467      469       +2     
Flag Coverage Δ
os_linux 79.54% <90.35%> (+0.24%) ⬆️
os_macos 74.15% <90.35%> (+0.34%) ⬆️
os_windows 80.73% <90.35%> (+0.22%) ⬆️
unit 80.86% <90.35%> (+0.22%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

@aemengo aemengo marked this pull request as ready for review September 16, 2021 03:30
@aemengo aemengo requested a review from a team as a code owner September 16, 2021 03:30
@jromero jromero self-assigned this Sep 28, 2021
Copy link
Member

@importhuman importhuman left a comment

Choose a reason for hiding this comment

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

Hey @aemengo, this looks insanely cool! Had a couple of questions:

  • When starting the build, the initial steps show up in the terminal rather than the dashboard. Is this expected in certain cases? The mockup in the RFC opened up the dashboard immediately, hence my question. (I'd built the image with a Dockerfile before, so there's no pulling all the layers in the attached video)
  • I didn't get the detect phase in my dashboard. The 1st video is building an already existing image (though without changes to source code), and the 2nd is building a new image (same name as earlier, but I'd deleted the image before building).
  • In which cases will the plan be shown? I didn't get anything in either case.
  • It might be helpful to add a "Press control + c to exit dashboard" message after the build succeeds/fails, since this is meant to be beginner friendly, and also because some folks like me might not realize that the way to exit the dashboard is the same as stopping a running command.
Screen.Recording.2021-09-29.at.2.19.14.PM.mov
Screen.Recording.2021-09-29.at.2.28.58.PM.mov

@aemengo
Copy link
Contributor Author

aemengo commented Sep 29, 2021

@importhuman

When starting the build, the initial steps show up in the terminal rather than the dashboard.

Yes, it's expected. Doing the mockup, exactly, was too hard 😕. For now I'm happy to compromise with how it is now.

I didn't get the detect phase in my dashboard.

That was also intended. I thought it looked better that way because the plan is supposed to show the same information. But, I don't feel strongly about this at all.

In which cases will the plan be shown?

This is definitely not intended. It's always supposed to show. Could be a bug.
The regex is supposed to match (\S+\/\S+)\s+([\d\.]+). Maybe it's because of the lifecycle version? What does the DETECT phase logs say without --interactive?

It might be helpful to add a "Press control + c to exit dashboard" message

I think this is a great idea 🙌🏾
If you have a vision for how it should look, maybe you can draft the issue for it.

this looks insanely cool!

Just you wait. It's gonna be even cooler! 😁

@importhuman
Copy link
Member

Ah, now that I think about it the mockup would be before adding the feature itself.

What does the DETECT phase logs say without --interactive?

===> DETECTING
3 of 6 buildpacks participating
google.go.runtime  0.9.1
google.go.build    0.9.0
google.utils.label 0.0.1

@aemengo
Copy link
Contributor Author

aemengo commented Sep 29, 2021

@importhuman Thanks for the providing the output, It was definitely a bug. I wasn't expecting the output to look like that.
Should be: Fixed in latest commit 👍🏾

@jromero jromero modified the milestones: 0.22.0, 0.23.0 Nov 2, 2021
Copy link
Member

@dfreilich dfreilich left a comment

Choose a reason for hiding this comment

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

In terms of code and experience, it's fabulous – thank you so much @aemengo!
Four small UX nits, but they can definitely be added in a future PR:

  1. I thought that I would be able to do something, while in the interactive mode, by selecting ENTER, or would be able to go into a separate pane, but it seems like I wasn't able to. Is the intention, ultimately, that there would be actions given?
  2. As a result of not being able to change windows, once Build logs scrolled past, I was unable to see them by scrolling up in my terminal, though maybe that's my iterm settings
  3. I think having a clear commands window, at the least saying Ctrl+c to close the interactive window, would be great
  4. It wasn't initially intuitive that when I exited out of the interactive window, that the build was still continuing behind the scene. That's ok, but I do think signaling that to the user (maybe by printing a message out in the logger when they close the interactive window, that the build is still happening and they can exit out of it by pressing Ctrl+c), would make it much clearer for the users.

What do you think of that?

Regardless, this is really great work, and I'm happy to merge it in, once the merge conflicts are resolved

@aemengo
Copy link
Contributor Author

aemengo commented Nov 11, 2021

@dfreilich Thanks for reviewing this PR. I really appreciate the time and effort. Hopefully the merge conflicts should be all resolved now. 👍🏾

  1. I thought that I would be able to do something, while in the interactive mode, by selecting ENTER [...] Is the intention, ultimately, that there would be actions given?

Yes, this is the intention. I have a follow-up branch to this PR that will introduce a "dive" panel but delineated by buildpack. Here's an example:

Screen Shot 2021-11-10 at 10 47 54 PM

  1. As a result of not being able to change windows, once Build logs scrolled past, I was unable to see them by scrolling up in my terminal, though maybe that's my iterm settings

This will also be fixed in the follow-up branch mentioned above.

  1. I think having a clear commands window, at the least saying Ctrl+c to close the interactive window, would be great

I share the sentiment, but I simply haven't come up with the UX yet. It will definitely come up in a subsequent PR though.

  1. It wasn't initially intuitive that when I exited out of the interactive window, that the build was still continuing behind the scene. [...].

That's something I didn't think of.. Would definitely like to address this is in a subsequent PR.

Signed-off-by: Anthony Emengo <aemengo@vmware.com>
@dfreilich dfreilich merged commit 8626649 into buildpacks:main Nov 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/enhancement Issue that requests a new feature or improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants