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

switch machine controller tests that use SSA to envtest and drop disableNodeLabelSync #7964

Closed
ykakarap opened this issue Jan 20, 2023 · 7 comments · Fixed by #8044
Closed
Assignees
Labels
area/testing Issues or PRs related to testing triage/accepted Indicates an issue or PR is ready to be actively worked on.
Milestone

Comments

@ykakarap
Copy link
Contributor

Detailed Description

The machine controller disables portions of its reconciliation logic that depend on SSA by setting disableNodeLabelSync to true during tests. This is because the current test setup for the machine controller uses fake client which does not support SSA.

This issue to switch over the tests, at least the ones that need a working SSA, to envtest and dropping the disableNodeLabelSync from the machine reconciler.

/kind test

@k8s-ci-robot
Copy link
Contributor

@ykakarap: The label(s) kind/test cannot be applied, because the repository doesn't have them.

In response to this:

Detailed Description

The machine controller disables portions of its reconciliation logic that depend on SSA by setting disableNodeLabelSync to true during tests. This is because the current test setup for the machine controller uses fake client which does not support SSA.

This issue to switch over the tests, at least the ones that need a working SSA, to envtest and dropping the disableNodeLabelSync from the machine reconciler.

/kind test

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@k8s-ci-robot k8s-ci-robot added the needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. label Jan 20, 2023
@ykakarap
Copy link
Contributor Author

/area testing

@k8s-ci-robot k8s-ci-robot added the area/testing Issues or PRs related to testing label Jan 21, 2023
@sbueringer
Copy link
Member

sbueringer commented Jan 23, 2023

Not sure if it is implied or not.

Let's please also consider splittung up the code so we would only have to implement a small part of our tests with envtest (and to improve code structure)

All the pre-existing tests that have nothing to do with node label sync don't necessary have to be migrated to envtest.

@sbueringer
Copy link
Member

/triage accepted

@k8s-ci-robot k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Jan 23, 2023
@ykakarap
Copy link
Contributor Author

/milestone v1.4

@k8s-ci-robot
Copy link
Contributor

@ykakarap: You must be a member of the kubernetes-sigs/cluster-api-maintainers GitHub team to set the milestone. If you believe you should be able to issue the /milestone command, please contact your Cluster API Maintainers and have them propose you as an additional delegate for this responsibility.

In response to this:

/milestone v1.4

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@sbueringer sbueringer added this to the v1.4 milestone Jan 26, 2023
@sbueringer
Copy link
Member

/assign

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/testing Issues or PRs related to testing triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants