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

Refactoring: Remove localClient global Variable in remote pkg #1126

Closed
3 tasks
jeremyharisch opened this issue Dec 6, 2023 · 0 comments · Fixed by #1426
Closed
3 tasks

Refactoring: Remove localClient global Variable in remote pkg #1126

jeremyharisch opened this issue Dec 6, 2023 · 0 comments · Fixed by #1426
Assignees
Labels
area/quality Related to all activites around quality kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt.
Milestone

Comments

@jeremyharisch
Copy link
Contributor

jeremyharisch commented Dec 6, 2023

Description

To have some of our code testable in EnvTest, we introduced a global variable called localClient in the remote package.

But, we should not have production ready code including code which is just used for testing, instead the code should be written in a way that it is testable without such modifications.

The global var localClient is set during EnvTests when creating a new SKRCluster, and thus when the LifecycleManager tries to resolve the SKRClient, it instead used the set localClient. With the current implementation each test scenario using the NewSKRCluster func runs in its own virtual SKR cluster, it would be great to keep such behaviour to have separated test cases.

The initializeKymaContext is a standalone function, but it could be a method on a new i.e. RemoteClientFactory (explicit deps on Kyma reconciler; new interface). The dependency could be replaced in the envTest with a Factory returning the localClient instead of SKR Client.

Reasons

  • Not have test code in production.
  • Reduce tech dept.
  • Avoided unwanted behaviour by overwriting the global variable.

Acceptance Criteria

  • Refactor the implementation of restConfigFromStrategy in such a way that it does not use a global variable to determine if a localClient should be used
  • Adapt test suits
  • Test cases should still run in own virtual SKR cluster

Feature Testing

Integration tests

Testing approach

No response

Attachments

No response

@jeremyharisch jeremyharisch added the kind/feature Categorizes issue or PR as related to a new feature. label Dec 6, 2023
@janmedrek janmedrek added area/quality Related to all activites around quality kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. and removed kind/feature Categorizes issue or PR as related to a new feature. labels Dec 15, 2023
@lindnerby lindnerby assigned lindnerby and unassigned ruanxin Feb 26, 2024
@janmedrek janmedrek added this to the 0.1.3 milestone Mar 26, 2024
@lindnerby lindnerby linked a pull request May 6, 2024 that will close this issue
@lindnerby lindnerby removed their assignment May 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/quality Related to all activites around quality kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants