chore: remove the need of passing rest config #895
Conversation
By removing reference to non-existing func. This function has been in use outside of this component.
As we are already using client.Client interface we do not have to instantiate other typed clients to e.g. list resources using their own funcs. Generic client.Client is sufficient for these needs. Additionally this change adds ctx propogation for these calls.
| @@ -37,7 +37,6 @@ can be found [here](https://github.com/opendatahub-io/opendatahub-operator/tree/ | |||
| GetManagementState() operatorv1.ManagementState | |||
| SetImageParamsMap(imageMap map[string]string) map[string]string | |||
| UpdatePrometheusConfig(cli client.Client, enable bool, component string) error | |||
| WaitForDeploymentAvailable(ctx context.Context, r *rest.Config, c string, n string, i int, t int) error | |||
There was a problem hiding this comment.
I would challenge the usefulness of keeping this code snippet if it's not automatically sourced. It's been already outdated (but looks way nicer this way! simple!)
There was a problem hiding this comment.
there is a ticket on update "docs"
this should be part of that task.
There was a problem hiding this comment.
Does it mean it will be automated? For example asciidoc with Antora toolchain makes it a breeze.
bartoszmajsak
changed the title
bartoszmajsak
changed the title
| @@ -169,7 +167,7 @@ func (d *Dashboard) ReconcileComponent(ctx context.Context, | |||
| if platform == deploy.ManagedRhods { | |||
| if enabled { | |||
| // first check if the service is up, so prometheus won't fire alerts when it is just startup | |||
| if err := monitoring.WaitForDeploymentAvailable(ctx, resConf, ComponentNameSupported, dscispec.ApplicationsNamespace, 20, 3); err != nil { | |||
| if err := monitoring.WaitForDeploymentAvailable(ctx, cli, ComponentNameSupported, dscispec.ApplicationsNamespace, 20, 3); err != nil { | |||
There was a problem hiding this comment.
20, 3 - do we really need that configurable for every component? I would assume most just copy-paste the values. Can't we const it and simplify this function?
| @@ -80,7 +79,7 @@ type ManifestsConfig struct { | |||
| } | |||
|
|
|||
| type ComponentInterface interface { | |||
| ReconcileComponent(ctx context.Context, cli client.Client, resConf *rest.Config, owner metav1.Object, DSCISpec *dsciv1.DSCInitializationSpec, currentComponentStatus bool) error | |||
| ReconcileComponent(ctx context.Context, cli client.Client, owner metav1.Object, DSCISpec *dsciv1.DSCInitializationSpec, currentComponentStatus bool) error | |||
There was a problem hiding this comment.
I find it a bit puzzling to have currentComponentStatus bool as a contract to all components where it's only used for one component and one case and others just _ ignore it. I think we should simplify it.
There was a problem hiding this comment.
i agree with you.
it was a bit hard to understand when it was introduced, we can have a follow up PR
There was a problem hiding this comment.
We came up with the fix with @aslakknutsen - see #896
| func getClusterServiceVersion(ctx context.Context, c client.Client, watchNameSpace string) (*ofapi.ClusterServiceVersion, error) { | ||
| clusterServiceVersionList := &ofapi.ClusterServiceVersionList{} | ||
| if err := c.List(ctx, clusterServiceVersionList, client.InNamespace(watchNameSpace)); err != nil { | ||
| return nil, fmt.Errorf("failed listign cluster service versions: %w", err) |
There was a problem hiding this comment.
| return nil, fmt.Errorf("failed listign cluster service versions: %w", err) | |
| return nil, fmt.Errorf("failed listing ClusterServiceVersions: %w", err) |
|
[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 |
* chore: fixes ComponentInterface docs By removing reference to non-existing func. This function has been in use outside of this component. * fix: removes rest config As we are already using client.Client interface we do not have to instantiate other typed clients to e.g. list resources using their own funcs. Generic client.Client is sufficient for these needs. Additionally this change adds ctx propogation for these calls.
* chore: fixes ComponentInterface docs By removing reference to non-existing func. This function has been in use outside of this component. * fix: removes rest config As we are already using client.Client interface we do not have to instantiate other typed clients to e.g. list resources using their own funcs. Generic client.Client is sufficient for these needs. Additionally this change adds ctx propogation for these calls. (cherry picked from commit 2ec8958)
As we are already using
client.Clientinterface we do not have to instantiate other typed clients to e.g. list resources using their ownfuncs. Generic
client.Clientis sufficient for these needs.Additionally this change adds ctx propogation for these calls.