fix: inspects owning DSC to determine installation #896
Conversation
bartoszmajsak
commented
Mar 6, 2024
•
- simplifies ComponentInterface by removing extra parameter in component reconcile (used only for dashboard)
- uses unstructured object to inspect DSC as it cannot import this struct due to cycle
- adds unit tests
d614d7a to
3866a4e
Compare
| @@ -281,3 +287,30 @@ func (d *Dashboard) cleanOauthClient(cli client.Client, dscispec *dsciv1.DSCInit | |||
| } | |||
| return nil | |||
| } | |||
|
|
|||
| func (d *Dashboard) IsInstalled(owner metav1.Object) (bool, error) { | |||
There was a problem hiding this comment.
I can be totally wrong, but at the first glance it looks like other way around.
I mean, DataScienceCluster object way down inside operator code serves as owner only in deploy in SetControllerReference(), so ReconcileComponent() and below can take it, then the method will be DataScienceCluster.IsInstalled(componentName string) or (component ComponentInterface)
There was a problem hiding this comment.
It can't take DataScienceCluster type because of the package import cycle limitation of golang (DSC already imports component package). I assume that is why it's defined as super type metav1.Object as it's not documented why the owner is needed.
Lets break it down. Original contract was:
ReconcileComponent(ctx context.Context, cli client.Client, resConf *rest.Config, owner metav1.Object, DSCISpec *dsciv1.DSCInitializationSpec, currentComponentStatus bool)
This is called from the reconcile as follows:
We are passing instance (of DSC) as an owner (and that is the only owner used AFAIK) and then we pass the value of instance.Status.InstalledComponents[componentName]
My line of thinking here is that instead of polluting the API with that last boolean flag we could move this logic down to the only component which is now in need of this information. And in that method simply do the instance.Status.InstalledComponents[componentName] lookup.
There was a problem hiding this comment.
Though maybe func (d *Dashboard) IsInstalled(owner metav1.Object) (bool, error) naming is confusing here. I will think about it.
There was a problem hiding this comment.
@ykaliuta does it clear the confusion or am I still on a different train of thought? :)
There was a problem hiding this comment.
I would not say I was confused, just had a bit different view on design, so go ahead :)
There was a problem hiding this comment.
If this is confusing, then maybe what I'm trying to do here is wrong. I limit the owner to be DSC but is this limitation a good path forward? WDYT @zdtsw @VaishnaviHire ?
There was a problem hiding this comment.
i am not sure i understand this confusion.
is the confusion coming from the name of the method? or why we should have this method ? or a method to DSC instead of a generic function?
There was a problem hiding this comment.
i am not sure i understand this confusion. is the confusion coming from the name of the method? or why we should have this method ? or a method to DSC instead of a generic function?
A method to component instead of a method to DSC.
My way of thinking was: if component stored its own status or reference to the owner, than asking the status with "if I am installed()" sounds fine. But asking "if I am installed to (some owner)" is a bit clumsy. While asking DSC then "do you have installed(component)" more natural.
But the proposal fine for me, no problem (and as described other way won't work).
|
all in all, cannot we have this cleanOauthClient() be done as part of upgrade? |
not directly to this PR's change, but i wonder why we use installcomponents to add extra logic, which should be done in the upgrade along with other "odd" hanging resources |
I generally found it strange in the first place, but my goal here is to tidy it up a bit. As for why this is done the way it is - #712 left it unanswered? |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: zdtsw The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
- simplifies ComponentInterface by removing extra parameter in component reconcile (used only for dashboard) - uses unstructured object to inspect DSC as it cannot import this struct due to cycle - adds unit tests
3866a4e to
36fef9d
Compare
|
New changes are detected. LGTM label has been removed. |
| @@ -177,7 +177,7 @@ run: manifests generate fmt vet ## Run a controller from your host. | |||
| go run ./main.go | |||
|
|
|||
| .PHONY: image-build | |||
| image-build: # unit-test ## Build image with the manager. | |||
| image-build: ## Build image with the manager. | |||
There was a problem hiding this comment.
yes, sure. probably a mistaken in the old PR to commented this out
|
/retest-required |
|
PR needs rebase. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
@bartoszmajsak: The following tests failed, say
Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
|
refresh my memory , do we still want this PR? |
|
@bartoszmajsak Just following up, do we still need this PR? |
|
In the light of ongoing refactoring I guess you don't? |