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

Add new 'building' phase to workspaces #9453

Merged
merged 1 commit into from
Apr 27, 2022
Merged

Conversation

andrew-farries
Copy link
Contributor

@andrew-farries andrew-farries commented Apr 21, 2022

Description

Add a new building phase to the list of phases that a workspace passes through on its way to running. A workspace is in building iff its workspace docker image is being built.

Related Issue(s)

Part of #9410

How to test

Refactoring that should have no effect on behaviour:

  • Visit the preview environment for this PR
  • Add a new project and start a workspace from it.
  • The workspace starts up as normal.

Release Notes

NONE

Documentation

None

  • /werft with-vm=true

@andrew-farries andrew-farries changed the title Af/add new workspace phase Add new 'building' phase to workspaces Apr 21, 2022
@geropl geropl added the team: webapp Issue belongs to the WebApp team label Apr 21, 2022
@geropl geropl self-assigned this Apr 21, 2022
@andrew-farries
Copy link
Contributor Author

Imagebuilds are currently failing in the preview environment for this branch, but I don't think it's related to these changes:

image

image

@andrew-farries
Copy link
Contributor Author

andrew-farries commented Apr 22, 2022

All comments addressed @geropl. Still two outstanding issues that stop me having full confidence in this:

  • Imagebuilds are failing in the preview environment (see this comment)
  • tsserver reports errors on every usage of the new building phase, eg:
    image
    This doesn't stop the build, but is consistent between workspace restarts 🤔

@svenefftinge
Copy link
Member

The tsserver error goes away when you rebuild the gitpod-protocol and restart the TypeScript Language Server (VS Code command).

@geropl
Copy link
Member

geropl commented Apr 25, 2022

Imagebuilds are currently failing in the preview environment for this branch, but I don't think it's related to these changes:

@andrew-farries This is a recent bug we saw with preview-environments. I recommend trying to 1) rebase on main and 2) use with-vm=true as werft parameter.

tsserver reports errors on every usage of the new building phase, eg:

If you do yarn watch in components/server, for instance, it should rebuild all direct and transitive dependencies. Sometimes you have to restart tsserver manually, though, because it does not catch up all changes from fs.

@meysholdt
Copy link
Member

hey, can you please rebase your branch before you continue working with your preview env? The old base contains a ws-daemon that logs expensively.

To save costs, I'll manually remove the old preview env of this branch. Apologies for the inconvenience.

@andrew-farries andrew-farries force-pushed the af/add-new-workspace-phase branch from c00f147 to 28617ff Compare April 25, 2022 15:58
@andrew-farries
Copy link
Contributor Author

andrew-farries commented Apr 25, 2022

/werft run with-clean-slate-deployment=true

👍 started the job as gitpod-build-af-add-new-workspace-phase.14
(with .werft/ from main)

@andrew-farries
Copy link
Contributor Author

@meysholdt the Werft build for the branch fails after the rebase (with a clean slate deployment). Looks like its timing out trying to delete the staging-af-add-new0028eb5a0f namespace.

@andrew-farries andrew-farries force-pushed the af/add-new-workspace-phase branch 2 times, most recently from 927fcd1 to 9463f6f Compare April 26, 2022 13:22
@geropl
Copy link
Member

geropl commented Apr 27, 2022

Looking into the preview issues here 👀

@geropl geropl force-pushed the af/add-new-workspace-phase branch from 9463f6f to 1ddc7cc Compare April 27, 2022 09:37
@geropl
Copy link
Member

geropl commented Apr 27, 2022

/werft run

👍 started the job as gitpod-build-af-add-new-workspace-phase.18
(with .werft/ from main)

@geropl
Copy link
Member

geropl commented Apr 27, 2022

@andrew-farries If you could fix this comment we're good to go! Tests looked good otherwise, code as well! 👍

@andrew-farries andrew-farries force-pushed the af/add-new-workspace-phase branch from 1ddc7cc to 3150c52 Compare April 27, 2022 15:43
@andrew-farries andrew-farries marked this pull request as ready for review April 27, 2022 15:46
@andrew-farries andrew-farries requested review from a team April 27, 2022 15:46
Copy link
Member

@geropl geropl left a comment

Choose a reason for hiding this comment

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

Tested and works, code LGTM! 🙏

Copy link
Contributor

@mustard-mh mustard-mh left a comment

Choose a reason for hiding this comment

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

Code looks good.

@akosyakov But we will never access to other phases...

  • Gateway only opened if workspace phase is runing
  • Gateway will exit once we click Stop Workspace in browser

Why we need this? Is it a preparation for the future(like create workspace without browser)?

@roboquat roboquat merged commit 35e2178 into main Apr 27, 2022
@roboquat roboquat deleted the af/add-new-workspace-phase branch April 27, 2022 19:21
@roboquat roboquat added the deployed: IDE IDE change is running in production label Apr 27, 2022
@akosyakov
Copy link
Member

@mustard-mh you can connect on any phase from GW ui already now. In the future we would like to avoid going to browser at all.

@geropl
Copy link
Member

geropl commented Apr 28, 2022

@akosyakov @mustard-mh Please note: this is just adding the type, and adding forward-compatible handlers (e.g., treating it as equal to "preparing"). We'll wait with the follow up PR to actually enable it once we switched all clients. W

@roboquat roboquat added deployed: webapp Meta team change is running in production deployed Change is completely running in production labels Apr 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deployed: IDE IDE change is running in production deployed: webapp Meta team change is running in production deployed Change is completely running in production release-note-none size/M team: IDE team: webapp Issue belongs to the WebApp team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants