Only AggregatesController modifies aggregates and exposes Aggregates UUIDs#247
Only AggregatesController modifies aggregates and exposes Aggregates UUIDs#247
Conversation
fwiesel
force-pushed
the
cortex-aggregates
branch
3 times, most recently
from
2042fca to
5611895
Compare
fwiesel
changed the title
|
|
||
| // +kubebuilder:default:={} | ||
| // The UUIDs of the aggregates are used to apply aggregates to the hypervisor. | ||
| AggregateUUIDs []string `json:"aggregateUUIDs,omitempty"` |
There was a problem hiding this comment.
It would make much more sense to change the type of Aggregates to an array of tuples containing the aggregate name and id. Or alternatively, a map that uses uuid as a key.
There was a problem hiding this comment.
It would be a breaking change though.
There was a problem hiding this comment.
Okay, not only would it be a breaking change, but also would mean, we cannot simply compare .Status.Aggregates vs .Spec.Aggregates, but first collect all the values.
Cortex would have to do something similar. It is only interested in the uuids.
While I agree, that they are not independent values, and uuids and names belong together, I am not sure if that change would be net positive.
There was a problem hiding this comment.
I've opened #251, which changes aggregates in the status to be a struct with both name and uuid.
There was a problem hiding this comment.
yeah although I get it because of backwards compatibility (but not so important), I think it's just better to bite the sour apple.
fwiesel
force-pushed
the
cortex-aggregates
branch
2 times, most recently
from
67a8581 to
800ed63
Compare
Almost the same logic code was used in the aggregates and the onboarding controller, with the variation that the onboarding controller was also creating the zone aggregate, and the aggregates controller kept aggregates modified outside of the controller untouched. Let's drop that functionality and expect that all aggregates are somehow created externally (i.e. by another controller), and then we can combine that logic in one function, that also can be called by the offboarding controller. Cortex wants to rely on the aggregates in the status, so we keeping extra aggregates is unsupported either way.
For Cortex, we want the uuids in addition to the names, so keep track of them in the status. Let's keep only the aggregates controller changing aggregates.
fwiesel
force-pushed
the
cortex-aggregates
branch
from
800ed63 to
e063f6f
Compare
Merging this branch changes the coverage (1 decrease, 1 increase)
Coverage by fileChanged files (no unit tests)
Please note that the "Total", "Covered", and "Missed" counts above refer to code statements instead of lines of code. The value in brackets refers to the test coverage of that file in the old version of the code. Changed unit test files
|
|
Closed in favor of #251. |
2 participants
Uh oh!
There was an error while loading. Please reload this page.