Alpha module list#2884
Conversation
✅ Proposed changes verification passedThis pull request comes with up-to-date documentation and no illegal standard output usages. Find more detailed information in the |
✅ Proposed changes verification passedThis pull request comes with up-to-date documentation and no illegal standard output usages. Find more detailed information in the |
✅ 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 &moduleInstallationStateRepository{kubeClient: kubeClient} | ||
| } | ||
|
|
||
| func (r *moduleInstallationStateRepository) GetInstallationState(ctx context.Context, module entities.ModuleInstallation) (string, error) { |
There was a problem hiding this comment.
This is implementation of a separate repository for a single string value that I believe should live in the ModuleInstallation as the installation state belongs to the moduleInstallation. I see however that there's lot of extraction logic that might bloat the original repository.
I think we could introduce some kind of extractors or converters package that could live in the repository package that could have specialized classes responsible for such data transformations
There was a problem hiding this comment.
Refactored. Extracted extractStateFromObject as a function responsible for state interpretation.
|
There's some discrepancy in how |
✅ Proposed changes verification passedThis pull request comes with up-to-date documentation and no illegal standard output usages. Find more detailed information in the |
✅ Proposed changes verification passedThis pull request comes with up-to-date documentation and no illegal standard output usages. Find more detailed information in the |
2 similar comments
✅ Proposed changes verification passedThis pull request comes with up-to-date documentation and no illegal standard output usages. Find more detailed information in the |
✅ Proposed changes verification passedThis pull request comes with up-to-date documentation and no illegal standard output usages. Find more detailed information in the |
…solveInstallationState
✅ Proposed changes verification passedThis pull request comes with up-to-date documentation and no illegal standard output usages. Find more detailed information in the |
✅ Proposed changes verification passedThis pull request comes with up-to-date documentation and no illegal standard output usages. Find more detailed information in the |
1 similar comment
✅ Proposed changes verification passedThis pull request comes with up-to-date documentation and no illegal standard output usages. Find more detailed information in the |
✅ Proposed changes verification passedThis pull request comes with up-to-date documentation and no illegal standard output usages. Find more detailed information in the |
| "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" | ||
| ) | ||
|
|
||
| type ModuleCRStateRepository interface { |
There was a problem hiding this comment.
I think this class and moduleInstallationState repositories should be a part of ModuleInstallationRepository because they're technically not repositories - repositories are tightly related to some entity and manage their lifecycles:
ModuleInstallationEntityis managed byModuleInstallationRepository- get/list/create/update/delete
There was a problem hiding this comment.
Done. ModuleCRStateRepository and ModuleInstallationStateRepository are now internal implementation details of ModuleInstallationsRepository
| "github.com/kyma-project/cli.v3/internal/modulesv2/entities" | ||
| ) | ||
|
|
||
| type InstalledModulesRepository interface { |
There was a problem hiding this comment.
I think the repository naming should reflect an association with the entity it's related to, so I would see it as ModuleInstallationsRepository
There was a problem hiding this comment.
Done — renamed to ModuleInstallationsRepository.
✅ Proposed changes verification passedThis pull request comes with up-to-date documentation and no illegal standard output usages. Find more detailed information in the |
✅ Proposed changes verification passedThis pull request comes with up-to-date documentation and no illegal standard output usages. Find more detailed information in the |
Description
Changes proposed in this pull request:
Related issue(s)
See also: