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

feat: Introduce workspace v1beta1 API #904

Merged
merged 3 commits into from
Mar 1, 2025
Merged

feat: Introduce workspace v1beta1 API #904

merged 3 commits into from
Mar 1, 2025

Conversation

Fei-Guo
Copy link
Collaborator

@Fei-Guo Fei-Guo commented Feb 28, 2025

Reason for Change:
This change introduces the v1beta1 version for workspace CRD. Note that ragengine is still in v1alpha1.

Requirements

  • added unit tests and e2e tests (if applicable).

Issue Fixed:

Notes for Reviewers:

This is a relatively big change but is straightforward mostly because we did not change any workspace CRD APIs between beta and alpha. The whole files under api/v1beta are directly copied from the api/v1alpha folder. The only important change is the line 181 in workspace_types.go to make v1beta1 as the CRD storage version.

There are some files shared by ragengine and workspace. This means some codes in v1beta1 fold are irrelevant to workspace crd. I will have a follow-up PR to clean up the irrelevant codes.

To accommodate the new CRD version, I switched to use v1beta in all workspace related controller/test code. Any code related to REGEngine API is unchanged (still use v1alpha1).

One important question is do we need to implement a conversion webhook to deal with different api versions of workspace CRs. Upon some manual testing, we found this is unnecessary. APIServer will automatically perform the conversion between v1beta1 and v1alpha1 and it works well when there are not fields changes between versions. This means the client can ask for a v1alpha1 or v1beta1 version of the object. The watcher can watch for both versions even when you install the watch for v1beta1 only. This largely simplifies the required code changes.

For all the documents, we will switch to use v1beta1 later (after the new release of v1beta1).

Copy link

codecov bot commented Feb 28, 2025

Codecov Report

Attention: Patch coverage is 52.58799% with 458 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
api/v1beta1/zz_generated.deepcopy.go 12.80% 243 Missing and 9 partials ⚠️
api/v1beta1/params_validation.go 54.73% 59 Missing and 27 partials ⚠️
api/v1beta1/workspace_validation.go 82.88% 51 Missing and 13 partials ⚠️
pkg/workspace/controllers/workspace_controller.go 46.93% 26 Missing ⚠️
api/v1beta1/labels.go 0.00% 14 Missing ⚠️
pkg/workspace/tuning/preset-tuning.go 41.66% 6 Missing and 1 partial ⚠️
pkg/workspace/manifests/manifests.go 78.94% 4 Missing ⚠️
pkg/utils/resources/nodes.go 0.00% 3 Missing ⚠️
api/v1beta1/workspace_default.go 0.00% 1 Missing ⚠️
pkg/workspace/inference/preset-inferences.go 83.33% 0 Missing and 1 partial ⚠️
Files with missing lines Coverage Δ
api/v1alpha1/workspace_types.go 100.00% <ø> (ø)
api/v1beta1/workspace_types.go 100.00% <100.00%> (ø)
pkg/utils/nodeclaim/nodeclaim.go 84.33% <100.00%> (-0.67%) ⬇️
pkg/utils/test/testUtils.go 100.00% <ø> (ø)
...kg/workspace/controllers/workspace_gc_finalizer.go 100.00% <100.00%> (ø)
pkg/workspace/controllers/workspace_status.go 93.47% <100.00%> (ø)
pkg/workspace/inference/template_inference.go 100.00% <100.00%> (ø)
pkg/workspace/webhooks/webhooks.go 0.00% <ø> (ø)
api/v1beta1/workspace_default.go 0.00% <0.00%> (ø)
pkg/workspace/inference/preset-inferences.go 75.60% <83.33%> (ø)
... and 8 more

... and 16 files with indirect coverage changes

Copy link
Collaborator

@zhuangqh zhuangqh left a comment

Choose a reason for hiding this comment

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

"APIServer will automatically perform the conversion between v1beta1 and v1alpha1 and it works well when there are not fields changes between versions. "
That's good.

Copy link
Collaborator

@bangqipropel bangqipropel left a comment

Choose a reason for hiding this comment

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

basically good for me.

Don't forget to change the unit-test in the next PR:
grep -v -e /vendor -e /api/v1alpha1/zz_generated.deepcopy.go -e /pkg/utils/test/...) \

@Fei-Guo
Copy link
Collaborator Author

Fei-Guo commented Mar 1, 2025

basically good for me.

Don't forget to change the unit-test in the next PR: grep -v -e /vendor -e /api/v1alpha1/zz_generated.deepcopy.go -e /pkg/utils/test/...) \

fixed

@Fei-Guo Fei-Guo merged commit 381f539 into main Mar 1, 2025
3 of 7 checks passed
@Fei-Guo Fei-Guo deleted the fguo-dev1 branch March 1, 2025 05:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants