Alpha module list#2884
Conversation
anoipm
added
the
do-not-merge/work-in-progress
✅ Proposed changes verification passedThis pull request comes with up-to-date documentation and no illegal standard output usages. Find more detailed information in the |
| return &installedModulesRepository{kymaClient: kymaClient} | ||
| } | ||
|
|
||
| func (r *installedModulesRepository) ListInstalledModules(ctx context.Context) ([]kyma.KymaModuleInfo, error) { |
There was a problem hiding this comment.
Here we leak the raw/low-level type kyma.KymaModuleInfo while we should return some entity related to the repository. It's InstalledModulesRepository so it suggest that it should return some kind of a module. Currently we don't have any Module entity in our domain. Also I wouldn't necessarily like the Module name as it's very general name in our case, the whole package is called Module. I was thinking about something like ModuleInstallation, we could have the ModuleInstallation entity + ModuleInstallationRepository
| if spec.CustomResourcePolicy == "CreateAndDelete" { | ||
| return status.State, nil | ||
| } | ||
|
|
||
| if spec.Managed != nil && !*spec.Managed { | ||
| return status.State, nil | ||
| } | ||
|
|
||
| moduleTemplate, err := r.kubeClient.Kyma().GetModuleTemplate(ctx, status.Template.GetNamespace(), status.Template.GetName()) | ||
| if err != nil { | ||
| if apierrors.IsNotFound(err) { | ||
| return "", nil | ||
| } | ||
| return "", errors.Wrapf(err, "failed to get ModuleTemplate %s/%s", status.Template.GetNamespace(), status.Template.GetName()) | ||
| } |
There was a problem hiding this comment.
Repositories should be responsible for mapping between the outside world (k8s cluster/database/some external rest service) and the inside world (our domain layer) and this code is some business logic responsible for the state calculation. This kind of code should be a part of service
| Version: module.Status.Version, | ||
| Channel: module.Status.Channel, | ||
| ModuleState: module.Status.State, | ||
| Managed: module.Spec.Managed == nil || *module.Spec.Managed, |
There was a problem hiding this comment.
we are lacking the proper Entities here and we are explicitly describing here some internal behavior or some internal business rule, I would imagine we should be able to have something like moduleInstallation.IsManaged()
2 participants
Description
Changes proposed in this pull request:
Related issue(s)
See also: