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.

Sign up for GitHub

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

Jump to bottom

[ws-daemon] Use baseserver to run its services #10005

Merged
merged 1 commit into from
May 30, 2022
Merged

[ws-daemon] Use baseserver to run its services #10005

merged 1 commit into from
May 30, 2022
+46 −161

Conversation

csweichel
Copy link
Contributor

@csweichel csweichel commented May 13, 2022 •
edited

Description

This PR bases ws-daemon on top of base-server. Doing so removes a nice amount of code and is a first step towards harmonising the initialisation of our services.

How to test

  • Ensure that ws-daemon comes up and remains running
  • Start a workspace and ensure that if initialises
  • Ensure ws-daemon still produces metrics

Release Notes

NONE

@csweichel csweichel force-pushed the cw/baseserver-wsdaemon branch 2 times, most recently from deb0917 to 098c273 Compare May 13, 2022 13:45
@csweichel csweichel force-pushed the cw/baseserver-cfg branch 4 times, most recently from 7e6cd6e to 1940499 Compare May 13, 2022 14:29
@csweichel csweichel force-pushed the cw/baseserver-cfg branch 2 times, most recently from 6c600ef to 1940499 Compare May 13, 2022 14:36
@csweichel csweichel mentioned this pull request May 13, 2022
Merged
@csweichel csweichel force-pushed the cw/baseserver-wsdaemon branch 4 times, most recently from ce94553 to 80069d3 Compare May 16, 2022 07:07
@csweichel csweichel marked this pull request as ready for review May 16, 2022 07:18
@csweichel csweichel requested a review from a team May 16, 2022 07:18
@github-actions github-actions bot added the team: workspace Issue belongs to the Workspace team label May 16, 2022
Base automatically changed from cw/baseserver-cfg to main May 16, 2022 07:53
@roboquat roboquat requested review from a team May 16, 2022 07:53
@roboquat roboquat added size/XXL and removed size/L labels May 16, 2022
@github-actions github-actions bot added team: delivery Issue belongs to the self-hosted team team: webapp Issue belongs to the WebApp team labels May 16, 2022
easyCZ
easyCZ reviewed May 16, 2022
View reviewed changes
components/ws-daemon/cmd/run.go Outdated Show resolved Hide resolved
components/ws-daemon/cmd/run.go Outdated Show resolved Hide resolved
components/ws-daemon/cmd/run.go Outdated Show resolved Hide resolved
install/installer/pkg/components/ws-daemon/constants.go
@@ -12,5 +14,5 @@ const (
HostBackupPath = "/var/gitpod/tmp/backup"
TLSSecretName = "ws-daemon-tls"
VolumeTLSCerts = "ws-daemon-tls-certs"
ReadinessPort = 8086
ReadinessPort = baseserver.BuiltinHealthPort
Copy link
Member

Choose a reason for hiding this comment

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

👍

components/ws-daemon/cmd/run.go Outdated Show resolved Hide resolved
jenting
jenting reviewed May 16, 2022
View reviewed changes
Copy link
Contributor

@jenting jenting left a comment

Choose a reason for hiding this comment

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

Could we rebase this PR on top of #9999?
Otherwise, the files changed contains the #9999 changed.

corneliusludmann
corneliusludmann approved these changes May 16, 2022
View reviewed changes
Copy link
Contributor

@corneliusludmann corneliusludmann left a comment

Choose a reason for hiding this comment

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

It seems that you need a self-hosted approval only for the go.mod and go.sum files in install/installer. To unblock this PR, I'll approve these changes on behalf of the self-hosted team and let Team WebApp do the final review of the actual changes.

@roboquat roboquat added size/L and removed size/XXL labels May 16, 2022
@easyCZ easyCZ mentioned this pull request May 17, 2022
Merged
@easyCZ easyCZ force-pushed the cw/baseserver-wsdaemon branch 3 times, most recently from 2c0b32d to 2d7e047 Compare May 24, 2022 08:41
@easyCZ
Copy link
Member

easyCZ commented May 24, 2022

I've now rebased, cleaned up and tested this. It's ready for review again. Adding hold just to give us some discussion time if needed.

/hold

@geropl
Copy link
Member

geropl commented May 30, 2022

@easyCZ What's left to be done to merge this? Especially from whom you expect to receive a review? (That's unclear to me at least from the history of this PR)

@easyCZ
Copy link
Member

easyCZ commented May 30, 2022

@geropl Code review. From team workspace who ultimately owns this component.

@geropl geropl requested a review from a team May 30, 2022 07:22
@jenting
Copy link
Contributor

jenting commented May 30, 2022 •
edited by werft-gitpod-dev-com bot

It's ready to go 🛹

/unhold

@roboquat roboquat merged commit 67ce7d0 into main May 30, 2022
@roboquat roboquat deleted the cw/baseserver-wsdaemon branch May 30, 2022 14:53
@roboquat roboquat added the deployed: webapp Meta team change is running in production label May 31, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@easyCZ easyCZ easyCZ left review comments

@corneliusludmann corneliusludmann corneliusludmann approved these changes

@jenting jenting jenting approved these changes

Assignees
No one assigned
Labels
deployed: webapp Meta team change is running in production release-note-none size/L team: delivery Issue belongs to the self-hosted team team: webapp Issue belongs to the WebApp team team: workspace Issue belongs to the Workspace team
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@csweichel @easyCZ @geropl @jenting @corneliusludmann @roboquat