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 desktop IDE image support in registry-facade and ws-manager #6364

Merged
merged 2 commits into from
Oct 26, 2021

Conversation

corneliusludmann
Copy link
Contributor

@corneliusludmann corneliusludmann commented Oct 22, 2021

Description

This PR has the parts of the umbrella PR #6270 that changes registry-facade and ws-manager to support desktop IDE images.

See #6270 (comment) for more info.

Release Notes

NONE

@roboquat roboquat requested review from aledbf and jldec October 22, 2021 16:25
@roboquat roboquat added team: workspace Issue belongs to the Workspace team size/XXL labels Oct 22, 2021
@codecov
Copy link

codecov bot commented Oct 22, 2021

Codecov Report

Merging #6364 (1fc23ff) into main (1d7c6ca) will increase coverage by 21.88%.
The diff coverage is 97.05%.

❗ Current head 1fc23ff differs from pull request most recent head 17967fb. Consider uploading reports for the commit 17967fb to get more accurate results
Impacted file tree graph

@@             Coverage Diff             @@
##             main    #6364       +/-   ##
===========================================
+ Coverage   19.04%   40.92%   +21.88%     
===========================================
  Files           2       36       +34     
  Lines         168     7381     +7213     
===========================================
+ Hits           32     3021     +2989     
- Misses        134     4126     +3992     
- Partials        2      234      +232     
Flag Coverage Δ
components-ee-ws-scheduler-app 63.95% <100.00%> (?)
components-ee-ws-scheduler-lib 64.12% <100.00%> (?)
components-image-builder-mk3-app 34.96% <100.00%> (?)
components-local-app-app-linux-amd64 ?
components-local-app-app-linux-arm64 ?
components-local-app-app-windows-386 ?
components-local-app-app-windows-amd64 ?
components-local-app-app-windows-arm64 ?
components-ws-manager-app 39.55% <95.45%> (?)
installer-app 6.08% <ø> (?)

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

Impacted Files Coverage Δ
components/ws-manager/pkg/manager/status.go 73.77% <93.33%> (ø)
components/ee/ws-scheduler/pkg/scaler/driver.go 54.42% <100.00%> (ø)
...image-builder-mk3/pkg/orchestrator/orchestrator.go 39.48% <100.00%> (ø)
components/ws-manager/pkg/manager/create.go 79.84% <100.00%> (ø)
components/local-app/pkg/auth/auth.go
components/local-app/pkg/auth/pkce.go
components/ws-manager/pkg/manager/manager.go 25.10% <0.00%> (ø)
installer/pkg/components/ws-manager/role.go 0.00% <0.00%> (ø)
components/ee/ws-scheduler/pkg/scaler/time.go 66.66% <0.00%> (ø)
... and 32 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1d7c6ca...17967fb. Read the comment docs.

Copy link
Contributor

@csweichel csweichel left a comment

Choose a reason for hiding this comment

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

Thank you very much for splitting this out. I'm somewhat surprised to see the change in this way, considering the original draft.

How much effort would it be to align this with the original design?

components/ws-manager-api/core.proto Outdated Show resolved Hide resolved
@roboquat roboquat added the team: webapp Issue belongs to the WebApp team label Oct 25, 2021
@corneliusludmann corneliusludmann force-pushed the clu/desktop-ide-ws-manager branch 2 times, most recently from 256e296 to afa6acd Compare October 25, 2021 14:15
Copy link
Contributor

@csweichel csweichel left a comment

Choose a reason for hiding this comment

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

Thank you for making that change.

We're not yet setting DeprecatedIdeImage when returning a WorkspaceSpec, that's backwards incompatible.

The remainder of changes could be put in place in a follow up PR.

components/ws-manager-api/core.proto Outdated Show resolved Hide resolved
components/ws-manager/pkg/manager/status.go Outdated Show resolved Hide resolved
@corneliusludmann corneliusludmann force-pushed the clu/desktop-ide-ws-manager branch 4 times, most recently from d1f9eae to 1fc23ff Compare October 26, 2021 12:06
@corneliusludmann
Copy link
Contributor Author

@csweichel It's done.

@csweichel
Copy link
Contributor

Awesome. Thank you for making those changes.

/lgtm

@roboquat
Copy link
Contributor

LGTM label has been added.

Git tree hash: dc4e900431f89b0442ce19bc69f1fce73ba40780

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.

server changes LGTM

@roboquat
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: csweichel, geropl

Associated issue: #6270

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@roboquat roboquat merged commit e7c93eb into main Oct 26, 2021
@roboquat roboquat deleted the clu/desktop-ide-ws-manager branch October 26, 2021 13:02
@roboquat roboquat added deployed: workspace Workspace team change is running in production deployed: webapp Meta team change is running in production deployed Change is completely running in production labels Oct 31, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved deployed: webapp Meta team change is running in production deployed: workspace Workspace team change is running in production deployed Change is completely running in production editor: jetbrains release-note-none size/XXL team: webapp Issue belongs to the WebApp team team: workspace Issue belongs to the Workspace team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants