This repository has been archived by the owner on Mar 16, 2024. It is now read-only.
Enable setting a default ComputeClass on projects #2470
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
njhale
force-pushed
the
enhance-project-default-cc
branch
6 times, most recently
from
58af11f
to
dd0ba07
Compare
njhale
force-pushed
the
enhance-project-default-cc
branch
4 times, most recently
from
5d2af27
to
0a52614
Compare
njhale
requested review from
thedadams,
keyallis,
iwilltry42,
g-linville and
tylerslaton
njhale
force-pushed
the
enhance-project-default-cc
branch
6 times, most recently
from
b4ef87e
to
1e305b1
Compare
g-linville
suggested changes
Feb 15, 2024
njhale
force-pushed
the
enhance-project-default-cc
branch
from
1e305b1
to
d3feb24
Compare
g-linville
approved these changes
Feb 15, 2024
thedadams
suggested changes
Feb 16, 2024
Comment on lines
+23
to
+27
| if cc := project.Spec.DefaultComputeClass; cc != "" && project.Status.DefaultComputeClass != cc { | ||
| // The spec has been changed, update the status field to match. | ||
| project.Status.DefaultComputeClass = cc | ||
| } |
There was a problem hiding this comment.
Similar to default and supported region(s), we should always have a status value for default compute class. Otherwise, the status field is not necessary.
| t := clusterComputeClass // Create a new variable that isn't being iterated on to get a pointer | ||
| defaultCCC = &t | ||
|
|
||
| // Create a new variable that isn't being iterated on to get a pointer |
There was a problem hiding this comment.
Is this comment still necessary?
Related: should we bump to Go 1.22?
Comment on lines
42
to
76
| if projectSpecifiedCCC != nil { | ||
| return projectSpecifiedCCC, nil | ||
| } |
There was a problem hiding this comment.
Spoke about this offline: if a user specifies a compute class that doesn't exist, then we should error here.
Additionally, we should check that the default region on the project is supported by the compute class.
Lastly, validation should be added for the user changing the default region on the project to ensure that the default compute class, if one is set, supports the region.
Comment on lines
77
to
102
| if projectSpecifiedPCC != nil { | ||
| return projectSpecifiedPCC, nil | ||
| } |
| defaultComputeClasses = append(defaultComputeClasses, ccc.Name) | ||
| } | ||
|
|
||
| if sets.New(defaultComputeClasses...).Has(projectSpecified) { |
There was a problem hiding this comment.
nit: slightly more performant.
Suggested change
| if sets.New(defaultComputeClasses...).Has(projectSpecified) { | |
| if slices.Contains(defaultComputeClasses, projectSpecified) { |
Comment on lines
38
to
62
| if _, err := computeclasses.GetAsProjectComputeClassInstance(ctx, v.Client, project.Name, defaultComputeClass); apierrors.IsNotFound(err) { | ||
| // The compute class does not exist, return an invalid error | ||
| result = append(result, field.Invalid(field.NewPath("spec", "defaultComputeClass"), defaultComputeClass, "default compute class does not exist")) | ||
| } else if err != nil { | ||
| // Some other error occurred while trying to get the compute class, return an internal error. | ||
| result = append(result, field.InternalError(field.NewPath("spec", "defaultComputeClass"), err)) | ||
| } | ||
| } |
There was a problem hiding this comment.
I left this comment elsewhere, but this is where we should ensure that the selected compute class supports the default region of the project.
njhale
force-pushed
the
enhance-project-default-cc
branch
2 times, most recently
from
1d70e3c
to
df43cfc
Compare
Signed-off-by: Nick Hale <4175918+njhale@users.noreply.github.com>
Signed-off-by: Nick Hale <4175918+njhale@users.noreply.github.com>
Signed-off-by: Nick Hale <4175918+njhale@users.noreply.github.com>
Add API-level validation of the defaultComputeClass spec field that requires a referenced compute class exist on project create or update. Signed-off-by: Nick Hale <4175918+njhale@users.noreply.github.com>
Add tests and update affected goldenfiles. Signed-off-by: Nick Hale <4175918+njhale@users.noreply.github.com>
Signed-off-by: Nick Hale <4175918+njhale@users.noreply.github.com>
Signed-off-by: Nick Hale <4175918+njhale@users.noreply.github.com>
Signed-off-by: Nick Hale <4175918+njhale@users.noreply.github.com>
Signed-off-by: Nick Hale <4175918+njhale@users.noreply.github.com>
Signed-off-by: Nick Hale <4175918+njhale@users.noreply.github.com>
njhale
force-pushed
the
enhance-project-default-cc
branch
from
df43cfc
to
f051f14
Compare
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Enable selecting the default
ComputeClassforAppsdeployed to a project via a new project spec field.The
DefaultComputeClassfield contains the name of a[Project|Cluster]ComputeClassInstanceto select as the default. If no compute classes with that name exist, the existingComputeClassdefaulting rules are applied.This PR also extends API validation for project create and update to ensure that a compute class with a matching name exists when the field is set.
Part of https://github.com/acorn-io/manager/issues/1943