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-manager-bridge] Remove HasMoreResources admission constraint #11384

Merged
merged 2 commits into from
Jul 15, 2022
Merged

[ws-manager-bridge] Remove HasMoreResources admission constraint #11384

merged 2 commits into from
Jul 15, 2022
+237 −161

Conversation

csweichel
Copy link
Contributor

@csweichel csweichel commented Jul 14, 2022 •
edited

Description

This PR removes the has-more-resources admission constraint. We have since started using workspace classes for this functionality, hence clean up this bit of code.

This PR is part of a stack of changes:

How to test

There should be no functional changes due to this removal.

Release Notes

NONE

Werft options:

  • /werft with-preview

@csweichel csweichel mentioned this pull request Jul 14, 2022
Merged
1 task
@csweichel csweichel mentioned this pull request Jul 14, 2022
Closed
1 task
@csweichel csweichel marked this pull request as ready for review July 14, 2022 14:53
@csweichel csweichel requested review from a team July 14, 2022 14:53
@github-actions github-actions bot added team: webapp Issue belongs to the WebApp team team: workspace Issue belongs to the Workspace team labels Jul 14, 2022
@geropl
Copy link
Member

geropl commented Jul 14, 2022

Looking at the diff is pointless because of the re-format. I assume it's a pure removal, so will wait for the preview env to do a basic test before approving.

@geropl
Copy link
Member

geropl commented Jul 14, 2022 •
edited by werft-gitpod-dev-com bot

/werft with-preview=true

👎 unknown command: with-preview=true
Use /werft help to list the available commands

@geropl
Copy link
Member

geropl commented Jul 14, 2022 •
edited by werft-gitpod-dev-com bot

/werft run with-preview=true

👍 started the job as gitpod-build-cw-remove-wsac-hmr.2
(with .werft/ from main)

Base automatically changed from cw/remove-wsac-usrlvl to main July 14, 2022 17:49
sagor999
sagor999 approved these changes Jul 14, 2022
View reviewed changes
Copy link
Contributor

@sagor999 sagor999 left a comment

Choose a reason for hiding this comment

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

Tested in preview env. Works.
Code LGTM, though formatting changes made it hard to review changes efficiently.

Offtopic: why so many formatting changes? Are we using different formatters across gitpod? 🤔

geropl
geropl approved these changes Jul 15, 2022
View reviewed changes
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.

Works! 👍

/hold Because the "user.featureFlag" has-more-resources could be removed as well, but is also fine as follow-up.

@csweichel
Copy link
Contributor Author

csweichel commented Jul 15, 2022 •
edited by werft-gitpod-dev-com bot

though formatting changes made it hard to review changes efficiently.

Offtopic: why so many formatting changes? Are we using different formatters across gitpod? 🤔

Totally agreed. It's a bit of nuisance because it also makes the PR look bigger than it is.

For the longest time we didn't use a formatter for TypeScript. When we introduced it recently, we didn't format all our code. Instead, when a file is saved now that we have the formatter, the formatting is applied.

@csweichel
[ws-manager-bridge] Remove HasMoreResources admission constraint
f26a8aa 
We've moved to workspace classes for this feature.
@csweichel
[ws-manager-api] Clean up "ExtendedUser"
96db6fd 
after the removal of the has-user-level and
has-more-resources admission constraint
@csweichel
Copy link
Contributor Author

csweichel commented Jul 15, 2022 •
edited

Because the "user.featureFlag" has-more-resources could be removed as well, but is also fine as follow-up.

I can't find the place in code that you refer to. Could you give me a pointer where I need to look?

@geropl
Copy link
Member

geropl commented Jul 15, 2022

Because the "user.featureFlag" has-more-resources could be removed as well, but is also fine as follow-up.
I can't find the place in code that you refer to. Could you give me a pointer where I need to look?

Sorry for the confusion: It's already gone but a leftover in DB state. Please ignore. 🧘

@geropl
Copy link
Member

geropl commented Jul 15, 2022

/unhold
☝️

@roboquat roboquat merged commit 65326d5 into main Jul 15, 2022
@roboquat roboquat deleted the cw/remove-wsac-hmr branch July 15, 2022 12:31
@roboquat roboquat added 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 labels Jul 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sagor999 sagor999 sagor999 approved these changes

@geropl geropl geropl approved these changes

@aledbf aledbf Awaiting requested review from aledbf

Assignees
No one assigned
Labels
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 release-note size/XL team: webapp Issue belongs to the WebApp team team: workspace Issue belongs to the Workspace team
Projects
No open projects
🌌 Workspace Team
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@csweichel @geropl @sagor999 @roboquat