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

[local-preview] Warn and Confirm from user before proceeding #13123

Merged
merged 1 commit into from
Sep 23, 2022

Conversation

Pothulapati
Copy link
Contributor

@Pothulapati Pothulapati commented Sep 20, 2022

Description

Currently, For Users with ARM CPU's, local-preview does not really work and its hard for the script to know as it runs inside docker (which could be a x86 VM).

This Updates the script to warn the users on the requirements before starting and running local-preview.

Screenshot 2022-09-20 at 6 27 14 PM

The default value is also YES which means even when the user presses enter, we proceed forward.

Signed-off-by: Tarun Pothulapati tarun@gitpod.io

Related Issue(s)

Fixes #11792

How to test

Run

docker run -p 443:443 --privileged --name gitpod -it  --mount type=volume,source=gitpod,destination=/var/gitpod eu.gcr.io/gitpod-core-dev/build/local-preview:tar-lp-warn-confirm.28

Release Notes

[local-preview] Warn and Confirm from user before proceeding

Documentation

Werft options:

  • /werft with-local-preview
  • /werft with-integration-tests=all
    Valid options are all, workspace, webapp, ide

Copy link
Contributor

@lucasvaltl lucasvaltl left a comment

Choose a reason for hiding this comment

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

LGTM (did not test or review the implementation), but suggested a content change that would be good to add.

@@ -38,6 +38,20 @@ var (
)

func main() {
// Warn and wait for user approval
pterm.FgBlue.Println(`
Welcome to the local preview of Gitpod. Please note the following limitations:
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd suggest we change this to:

Welcome to the local preview of Gitpod. Please note the following limitations:
  - Performance is limited by the capabilities of your machine - a minimum of 4 cores and 6GB of RAM are required
  - ARM CPUs including Macs with Apple Silicon (e.g. M1) are currently not supported
  For more information about these limitation, please visit the [local preview documentation](https://www.gitpod.io/docs/self-hosted/latest/local-preview).

--> This adds a link & removes the periods at the end of the bullet points.
--> I am assuming we can do links in the CLI output here, but not sure how / if this works.

Copy link
Contributor Author

@Pothulapati Pothulapati Sep 21, 2022

Choose a reason for hiding this comment

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

Screenshot 2022-09-21 at 11 31 13 AM

It is rendered like this, with the http link being clickable (depends on the shell too). WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

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

In that case we should likely not try to make the http link a clickable link (with the ... ) like on the web but instead have the link as full text:

Welcome to the local preview of Gitpod. Please note the following limitations:
  - Performance is limited by the capabilities of your machine - a minimum of 4 cores and 6GB of RAM are required
  - ARM CPUs including Macs with Apple Silicon (e.g. M1) are currently not supported
  For more information about these limitation, please visit the local preview documentation: https://www.gitpod.io/docs/self-hosted/latest/local-preview

Copy link
Contributor Author

@Pothulapati Pothulapati Sep 21, 2022

Choose a reason for hiding this comment

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

Updated. Will mark it ready once I test the final command out!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

While the input works with go run ., It does not seem to work as expected when we run it inside docker. So, Trying to make that work now!

@Pothulapati Pothulapati force-pushed the tar/lp-warn-confirm branch 4 times, most recently from 425a4f8 to 389afa8 Compare September 22, 2022 10:40
@roboquat roboquat added size/M and removed size/S labels Sep 22, 2022
@Pothulapati Pothulapati force-pushed the tar/lp-warn-confirm branch 2 times, most recently from 02ef87e to 0f9b73b Compare September 22, 2022 11:11
@Pothulapati
Copy link
Contributor Author

Pothulapati commented Sep 22, 2022

/werft run with-local-preview

👍 started the job as gitpod-build-tar-lp-warn-confirm.8
(with .werft/ from main)

@werft-gitpod-dev-com
Copy link

started the job as gitpod-build-tar-lp-warn-confirm.12 because the annotations in the pull request description changed
(with .werft/ from main)

@Pothulapati Pothulapati force-pushed the tar/lp-warn-confirm branch 2 times, most recently from 874072d to e3b964a Compare September 23, 2022 05:29
@Pothulapati
Copy link
Contributor Author

Pothulapati commented Sep 23, 2022

/werft run with-local-preview

👍 started the job as gitpod-build-tar-lp-warn-confirm.16
(with .werft/ from main)

@Pothulapati Pothulapati force-pushed the tar/lp-warn-confirm branch 8 times, most recently from 06edf01 to a5f5645 Compare September 23, 2022 07:49

r := io.TeeReader(os.Stdin, dmp)
file, err := tail.TailFile("logs.txt", tail.Config{Follow: true})
Copy link
Contributor Author

@Pothulapati Pothulapati Sep 23, 2022

Choose a reason for hiding this comment

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

@mrsimonemms Like we discussed in our call. This was because while logs.txt gets updated, We just read at a point and are exiting prettylog as there is EOF.

Instead we need a way to continuously read logs.txt as it gets updated, and this is where the tail package comes in . By using that we are continuously reading from the latest updates, and thus updating status based on that. There seems to be a bunch of projects that are already using tail, so feels pretty standard. 👍🏼

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Was also able to test that this works as expected!

@Pothulapati Pothulapati marked this pull request as ready for review September 23, 2022 08:18
@Pothulapati Pothulapati requested a review from a team September 23, 2022 08:18
@github-actions github-actions bot added the team: delivery Issue belongs to the self-hosted team label Sep 23, 2022
Currently, For Users with ARM CPU's, `local-preview` does not
really work and its hard for the script to know as it runs inside
docker (which could be a x86 VM).

This Updates the script to warn the users on the requirements before
starting and running `local-preview`.

Signed-off-by: Tarun Pothulapati <tarun@gitpod.io>
Copy link
Contributor

@mrsimonemms mrsimonemms left a comment

Choose a reason for hiding this comment

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

wORKS AS EXPECTED

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-note size/M team: delivery Issue belongs to the self-hosted team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[local preview] Check arch and fail fast on non-supported arch
4 participants