Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: Module catalog improvements implementation #1748

Merged
merged 7 commits into from
Aug 28, 2024
Merged

feat: Module catalog improvements implementation #1748

merged 7 commits into from
Aug 28, 2024

Conversation

Tomasz-Smelcerz-SAP
Copy link
Member

@Tomasz-Smelcerz-SAP Tomasz-Smelcerz-SAP commented Aug 7, 2024 •
edited
Loading

Description

Fixes #1591

Changes proposed in this pull request:

  • add support for explicit module version handling
  • ensure compatibility with existing logic
  • extend tests

Related issue(s)
#1589 , #1590

Compatibility
The following scenarios has been tested (pkg/templatelookup/regular_test.go):

  • installation of a module by version, then upgrade
  • installation of a module by version, then downgrade: Not allowed
  • change from channel to version (no upgrade)
  • change from channel to version (upgrade)
  • change from channel to version (downgrade): Not allowed
  • change from version to channel (no upgrade)
  • change from version to channel (upgrade)
  • change from version to channel (downgrade): Not allowed

@kyma-bot kyma-bot added cla: yes Indicates the PR's author has signed the CLA. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Aug 7, 2024
@Tomasz-Smelcerz-SAP Tomasz-Smelcerz-SAP changed the base branch from main to feat/module-catalogue-improvements August 7, 2024 05:58
@kyma-bot kyma-bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Aug 7, 2024
@Tomasz-Smelcerz-SAP Tomasz-Smelcerz-SAP force-pushed the feat/module-catalog-improvements-impl branch from 7894a11 to 4056f0b Compare August 8, 2024 06:48
@kyma-bot kyma-bot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Aug 8, 2024
@Tomasz-Smelcerz-SAP Tomasz-Smelcerz-SAP force-pushed the feat/module-catalog-improvements-impl branch from 3e62f0c to 1cf8ae0 Compare August 9, 2024 07:09
mmitoraj
mmitoraj previously approved these changes Aug 9, 2024
View reviewed changes
@kyma-bot kyma-bot added the lgtm Looks good to me! label Aug 9, 2024
ruanxin
ruanxin reviewed Aug 9, 2024
View reviewed changes
api/v1beta2/kyma_types.go Outdated Show resolved Hide resolved
ruanxin
ruanxin reviewed Aug 9, 2024
View reviewed changes
api/v1beta2/kyma_types.go Outdated Show resolved Hide resolved
ruanxin
ruanxin reviewed Aug 9, 2024
View reviewed changes
api/v1beta2/kyma_types.go Outdated Show resolved Hide resolved
ruanxin
ruanxin reviewed Aug 9, 2024
View reviewed changes
internal/controller/kyma/controller.go Outdated
r.Metrics.RecordRequeueReason(metrics.StatusSyncToRemote, queue.UnexpectedRequeue)
return newReconcileResult(r.requeueWithError(ctx, kyma, fmt.Errorf("could not synchronize remote kyma status: %w", sErr)))
}
return newReconcileResult(ctrl.Result{}, nil)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if you return ctrl.Result{}, nil, then this kyma will not reconcile anymore

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

update: I don't think you need to change this existing logic #1748 (comment)

ruanxin
ruanxin reviewed Aug 9, 2024
View reviewed changes
pkg/templatelookup/regular_test.go Outdated
WithModuleStatus(v1beta2.ModuleStatus{
Name: moduleToInstall.Name,
Channel: "regular",
Version: "1.1.0",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you define those versions for testing 1.1.0, 1.0.0 as a const value so that better for us to understand which field to test here?

Copy link
Member Author

@Tomasz-Smelcerz-SAP Tomasz-Smelcerz-SAP Aug 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought that just version numbers serve as a better "name" than the name itself - they're usually shorter and visually distinct. But If you think consts like "oldVersion" "newVersion" are better, I'll do that.

ruanxin
ruanxin requested changes Aug 9, 2024
View reviewed changes
Copy link
Contributor

@ruanxin ruanxin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

besides those comment, please also cover this missing AC in this one or followup issue.

The Lifecycle-Manager must support both the old ModuleTemplates and a new ones (with extended attributes) - we must provide a smooth migration path

@kyma-bot kyma-bot removed the lgtm Looks good to me! label Aug 9, 2024
@Tomasz-Smelcerz-SAP
Copy link
Member Author

Tomasz-Smelcerz-SAP commented Aug 12, 2024 •
edited
Loading

besides those comment, please also cover this missing AC in this one or followup issue.

The Lifecycle-Manager must support both the old ModuleTemplates and a new ones (with extended attributes) - we must provide a smooth migration path

There's a unit test with over 10 scenarios in pkg/templatelookup/regular_test.go.
It kind-of tests this AC: It tests if certain configuration of ModuleTemplates are allowed in the Spec. It doesn't test the installation, but the Module Catalog Improvement epic doesn't change much related to the installation process (and just to be sure I added a single e2e test for the installation).
Do you think we need to add another e2e tests just to cover the upgrade scenarios?

@kyma-bot kyma-bot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Aug 26, 2024
ruanxin
ruanxin reviewed Aug 27, 2024
View reviewed changes
pkg/templatelookup/availableModules.go Outdated
return !a.Enabled && shared.NoneChannel.Equals(a.Channel) && a.Version != ""
}

func GetAvailableModules(kyma *v1beta2.Kyma) []AvailableModule {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you refactor this GetAvailableModules not belong to kyma struct, I think it's better to rename it, not use GetXXX, it's not aligned with the naming conversion.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done.

ruanxin
ruanxin reviewed Aug 27, 2024
View reviewed changes
pkg/templatelookup/regular.go
return filteredTemplates[0], nil
}

func (t *TemplateLookup) getTemplateByVersion(ctx context.Context, name, version string) (
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The current implementation is fine, but we should plan a follow-up refactor to optimize the get template function. Instead of using List, we should retrieve the template directly by its moduleTemplate name. Since the moduleTemplate names follow a strict naming convention of [module name] + [channel or version], it will be straightforward to implement a Get request for direct fetching. This change will improve performance, especially when dealing with a large number of resources, as List is not efficient in such scenarios.

Can you create a followup issue?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#1805

@kyma-bot kyma-bot added the lgtm Looks good to me! label Aug 28, 2024
@kyma-bot kyma-bot merged commit c2f8f54 into kyma-project:feat/module-catalogue-improvements Aug 28, 2024
38 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ruanxin ruanxin ruanxin approved these changes

@mmitoraj mmitoraj mmitoraj left review comments

Assignees

@ruanxin ruanxin

@mmitoraj mmitoraj

Labels
cla: yes Indicates the PR's author has signed the CLA. lgtm Looks good to me! size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Module catalogue improvements] Implement new Module Catalog support
4 participants
@Tomasz-Smelcerz-SAP @ruanxin @mmitoraj @kyma-bot