Add default cluster pool assignments to config #600
Conversation
Signed-off-by: Katrina Rogan <katroganGH@gmail.com>
katrogan
requested review from
wild-endeavor,
EngHabu,
kumare3,
pmahindrakar-oss,
hamersaw and
eapolinario
as code owners
Codecov Report
@@ Coverage Diff @@
## master #600 +/- ##
==========================================
+ Coverage 58.66% 60.22% +1.56%
==========================================
Files 170 171 +1
Lines 16453 13443 -3010
==========================================
- Hits 9652 8096 -1556
+ Misses 5950 4496 -1454
Partials 851 851
Flags with carried forward coverage won't be shown. Click here to find out more.
|
| "github.com/flyteorg/flytestdlib/config" | ||
| ) | ||
|
|
||
| const clusterPoolsKey = "cluster_pools" |
There was a problem hiding this comment.
Dont we use camel case every where for the keys. Would be good to keep this consistent with other configs
There was a problem hiding this comment.
we're inconsistent :(
see https://github.com/flyteorg/flyteadmin/blob/master/flyteadmin_config.yaml#L165 and https://github.com/flyteorg/flyteadmin/blob/master/flyteadmin_config.yaml#L192
but I think the convention leans to camel case, updated
| clusterPoolAssignment := m.config.ClusterPoolAssignmentConfiguration().GetClusterPoolAssignments()[request.GetDomain()] | ||
|
|
||
| return &admin.ClusterAssignment{ | ||
| ClusterPoolName: clusterPoolAssignment.Pool, |
There was a problem hiding this comment.
What happens in case this comes as an empty pool name
There was a problem hiding this comment.
Existing behavior is preserved, we just don't assign a clusterpool which is fine.
| Pool string `json:"pool"` | ||
| } | ||
|
|
||
| type ClusterPoolAssignments = map[DomainName]ClusterPoolAssignment |
There was a problem hiding this comment.
Are these strictly going to be defined only per domain, what if we needed it also at project-domain level or is that not something we are considering for this
There was a problem hiding this comment.
Already implemented as a matchable attribute #376
TL;DR
Add default cluster pool assignments as application config options
Type
Are all requirements met?
Complete description
How did you fix the bug, make the feature etc. Link to any design docs etc
Tracking Issue
flyteorg/flyte#2248
Follow-up issue
NA