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

[ST] Create NamespaceManager for handling namespaces in the tests #9496

Merged
merged 4 commits into from Jan 4, 2024

Conversation

im-konge
Copy link
Member

@im-konge im-konge commented Jan 2, 2024

Type of change

  • Enhancement / Refactoring

Description

Currently we have creation and deletion of namespaces split across multiple classes, together with copying pull secrets and applying NetworkPolicies. This makes the work with STs complicated, as in case that we would like to change something in behavior of creation/deletion methods for the namespaces, we have to change it on multiple places.

Also, keeping the create/delete methods in the KubeClusterResource class makes things complicated - f.e. in case we would like to use methods from systemtest module inside the Namespace related methods, we cannot simply do that - as it would create cyclic dependency. One of the example is now, when UTO adds finalizers to KafkaTopics and we are deleting namespaces before deleting the KafkaTopics, the namespace deletion is stuck -> we have method in our systemtest module, which we cannot use in test module, so we would have to workaround it again.

This PR creates new class - NamespaceManager, deletes NamespaceUtils class as it's not needed anymore and does a small refactor.

Checklist

  • Make sure all tests pass

Signed-off-by: Lukas Kral <lukywill16@gmail.com>
Signed-off-by: Lukas Kral <lukywill16@gmail.com>
@im-konge im-konge added this to the 0.40.0 milestone Jan 2, 2024
@im-konge im-konge self-assigned this Jan 2, 2024
@im-konge
Copy link
Member Author

im-konge commented Jan 2, 2024

/azp run regression

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@im-konge
Copy link
Member Author

im-konge commented Jan 2, 2024

/azp run upgrade

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Signed-off-by: Lukas Kral <lukywill16@gmail.com>
Copy link
Member

@see-quick see-quick left a comment

Choose a reason for hiding this comment

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

LGTM. I have only one note that is worth consideration but not the scope of this PR. We should change the way how we store MAP_WITH_SUITE_NAMESPACES because, with the removal of class-wide parallelism, we do not need test class -> {namespaces...} we need only one dynamic object storing it.

@im-konge
Copy link
Member Author

im-konge commented Jan 3, 2024

LGTM. I have only one note that is worth consideration but not the scope of this PR. We should change the way how we store MAP_WITH_SUITE_NAMESPACES because, with the removal of class-wide parallelism, we do not need test class -> {namespaces...} we need only one dynamic object storing it.

Yeah I didn't really want to touch that logic, but I thought about it too. There is another thing -> that the NP are not really applied. But I don't want to mess more things together and I'll rather fix it in different ones.

Signed-off-by: Lukas Kral <lukywill16@gmail.com>
@im-konge im-konge marked this pull request as ready for review January 3, 2024 13:36
@im-konge
Copy link
Member Author

im-konge commented Jan 3, 2024

/azp run regression

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@im-konge
Copy link
Member Author

im-konge commented Jan 3, 2024

/azp list

@im-konge
Copy link
Member Author

im-konge commented Jan 3, 2024

/azp run namespace-rbac-scope-regression

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@im-konge im-konge merged commit bfa2e36 into strimzi:main Jan 4, 2024
29 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants