Skip to content

proposal: gp prebuild-logs Gitpod cli command #13505

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

Closed
wants to merge 2 commits into from

Conversation

Siddhant-K-code
Copy link
Member

@Siddhant-K-code Siddhant-K-code commented Oct 1, 2022

Important

Warning This PR is work in progress, and a proposal. It will not be merged without prior discussions, however please treat as low priority.


Signed-off-by: Siddhant Khare siddhant@gitpod.io

Description

This PR Introduces the new command for Gitpod CLI, gp prebuild-logs which will open the Prebuild Logs File (if it exists)

Related Issue(s)

Fixes #

How to test

  • Open preview dev environment
  • Open an empty workspace from empty repository from preview env
  • Open Terminal & Run gp preuibld-logs

Release Notes

feature: new gitpod cli command for prebuild logs - `gp prebuild-logs`

Documentation

Yes. (will update it, after merge)

Werft options:

  • /werft with-local-preview
    If enabled this will build install/preview
  • /werft with-preview
  • /werft with-integration-tests=all
    Valid options are all, workspace, webapp, ide

Signed-off-by: Siddhant Khare <siddhant@gitpod.io>
@Siddhant-K-code
Copy link
Member Author

Siddhant-K-code commented Oct 1, 2022

/werft run

👍 started the job as gitpod-build-feat-gitpod-cli-gp-prebuild-logs.2
(with .werft/ from main)

@svenefftinge
Copy link
Member

svenefftinge commented Oct 1, 2022

Would it make more sense for the command to print the prebuild logs to stdout instead of opening them in an editor?

@Siddhant-K-code
Copy link
Member Author

Would it make more sense for the command to print the prebuild logs to stdout instead of opening them in an editor?

Here is the scenario. The count of Prebuild Logs depends on the number of init tasks. So it might be not a good user flow to log out multiple files.

image

And, in some cases (mostly). Prebuild Logs are too long
image

@loujaybee
Copy link
Member

loujaybee commented Oct 3, 2022

This looks interesting, @Siddhant-K-code 👀 and was an idea that we also had to support:


Topic: choosing defaults

Would it make more sense for the command to print the prebuild logs to stdout instead of opening them in an editor? - @svenefftinge

  • I agree with Sven, we should pick sensible defaults. printing to stdout is useful for text searching tools w/ grep (should we consider a flag to open in the text editor, e.g. gp logs prebuild --edit?)
  • We are multi editor/IDE at Gitpod, let's be careful to not favour VS Code blindly. We should make sure our solution works with all the IDEs and for CLI users. That's one reason to favour printing to stdout as default / first iteration.

Topic: namespacing

Have we considered gp logs as the namespace? Then we can extend in future with...

  • gp logs dotfiles
  • gp logs prebuilds

...etc

  • I'd appreciate @andreafalzetti's input here
  • @csweichel has also raised the need to improve our standardards for our gp CLI args - which I do agree with

Topic: priority

Whilst promising, let's treat this PR as low(er) priority, so that the @gitpod-io/engineering-ide have appropriate time to give feedback and test the solution before merging. Related, but we should avoid PR's without issues, as we're discussing on an active PR, where ideally this could have been discussed as an issue to get more clarity on the solution first. 🙏

@Siddhant-K-code
Copy link
Member Author

Siddhant-K-code commented Oct 3, 2022

Related, but we should avoid PR's without issues, as we're discussing on an active PR, where ideally this could have been discussed as an issue to get more clarity on the solution first. 🙏

Ok, will remember that next time.
this is just the base of it because it was pending for a long time (& it was required) and there was no active discussion on it.
I have marked it as a draft, we can discuss the solution and will make the changes to it afterward.

@akosyakov
Copy link
Member

Although here is indeed many alternatives from user and implementation perspective. I wonder how we can skateboard which can be rolled back easily. How about contributing it only if latest version of IDE is selected and with experimental disclaimer in help. It does not need to be surfaced in docs and so on, but we could to use it internally or with customers via asking to choose latest and iterate.


import (
"fmt"
"github.com/gitpod-io/gitpod/gitpod-cli/pkg/theialib"
Copy link
Member

Choose a reason for hiding this comment

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

We should ditch theialib. The official api is supervisor API, maybe we should think about how enrich it to return task output. We were discussing such API in the past with @geropl that dashboard could as well return output of prebuild task even if some task already finished.

I think it would be in the right direction to make prebuild UX is more reliable. cc @csweichel


// initCmd represents the init command
var prebuildLogs = &cobra.Command{
Use: "prebuild-logs",
Copy link
Member

@akosyakov akosyakov Oct 4, 2022

Choose a reason for hiding this comment

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

How about gp prebuilds list + gp prebuilds log to list human readable names of tasks and then fetch content.

Although there is only on prebuild. I would think it should somehow resolve around tasks namespace or we need some new log namespace, for example:

> gp tasks list --prebuild

1. npm 
2. gradle

> gp tasks log 2 --prebuild

log output....

but I'm not sure what would the best UX

Copy link
Member Author

Choose a reason for hiding this comment

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

Currently prebuild-logs-X contains lot of things from binary characters to even process completion % (see picture here). That's why I was saying to avoid printing in terminal.

Copy link
Member

@akosyakov akosyakov Oct 4, 2022

Choose a reason for hiding this comment

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

I think they are special characters for shell and should be interpreted properly. We don't write anything else to these files besides what was produced by pty devices for shell.

`,
Run: func(cmd *cobra.Command, args []string) {

const prebuildFilePath = "/workspace/.gitpod/prebuild-log-*"
Copy link
Member

Choose a reason for hiding this comment

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

it is implementation detail of supervisor, i think it should be a secret of supervisor how to store and fetch content. Clients should not rely on it.

See https://en.wikipedia.org/wiki/Information_hiding

@loujaybee loujaybee changed the title feat: gp prebuild-logs Gitpod cli command experimental/proposal: gp prebuild-logs Gitpod cli command Oct 5, 2022
@loujaybee loujaybee changed the title experimental/proposal: gp prebuild-logs Gitpod cli command proposal: gp prebuild-logs Gitpod cli command Oct 5, 2022
@stale
Copy link

stale bot commented Oct 15, 2022

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the meta: stale This issue/PR is stale and will be closed soon label Oct 15, 2022
@stale stale bot closed this Oct 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants