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.

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

feat: Refactor template lookup to use strategy pattern #2200

Merged
merged 16 commits into from
Jan 23, 2025

Conversation

c-pius
Copy link
Contributor

@c-pius c-pius commented Jan 21, 2025

Description

Changes proposed in this pull request:

  • refactors the template lookup to use a strategy pattern for looking up the module template info
    • previously we had lot's of branching depending on the module metadata (e.g., old or new data, installed via version or channel)
    • now we have dedicated strategies for each of those cases
      • each strategy determines itself whether it applies or not
      • if it applies, it can look up the module template info based for the case it covers
    • the aim is to make it easier to adapt the cases in the future (add, remove, refine)

💡 Note to reviewer

  • this PR is huge (apologies), the actual changes are not that much though
  • focus on the changes in templatelookup
    • the existing functionality has been moved from regular.go to the strategies in moduletempalteinfolookup
    • the business logic is just copied, no new functionality has been added
  • the rest is boilerplate for setting up the strategies and adapting the changed method signatures in tests

Related issue(s)

@c-pius c-pius requested a review from a team as a code owner January 21, 2025 17:06
@c-pius c-pius added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 21, 2025
@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 Jan 21, 2025
@c-pius c-pius linked an issue Jan 22, 2025 that may be closed by this pull request
7 tasks
@c-pius c-pius removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 22, 2025
Copy link
Member

@lindnerby lindnerby left a comment

Choose a reason for hiding this comment

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

Nice job! Just left some mostly testing related suggestions 🙌

pkg/templatelookup/moduletemplateinfolookup/aggregated.go Outdated Show resolved Hide resolved
pkg/templatelookup/moduletemplateinfolookup/by_version.go Outdated Show resolved Hide resolved
pkg/templatelookup/regular_test.go Outdated Show resolved Hide resolved
pkg/templatelookup/regular_test.go Show resolved Hide resolved
pkg/module/sync/runner.go Show resolved Hide resolved
@lindnerby lindnerby enabled auto-merge (squash) January 23, 2025 14:19
@kyma-bot kyma-bot added the lgtm Looks good to me! label Jan 23, 2025
@lindnerby lindnerby merged commit d4ab059 into kyma-project:main Jan 23, 2025
67 of 69 checks passed
@c-pius c-pius deleted the feat/refactor-template-lookup branch January 23, 2025 14:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
Development

Successfully merging this pull request may close these issues.

[Maintenance Windows] Skip module upgrades during maintenance window
3 participants