[ST] Replace builders for internal clients with the ones from test-clients repository

[im-konge]

Type of change

• Enhancement
• Refactoring

Description

This PR changes the builders we used in STs until now with builders directly from the test-clients repository.
It should help with more things:

• keeping changes on one place, so other users of test-clients don't have to write their own Jobs or copy the builders from our repository
• having the code clear as much as possible
• code will be less confusing in terms of "what is happening" and "why it worked before and now it doesn't" - bright example can be NetworkPolicies being applied automatically for Bridge clients and then, when you are debugging something locally, you forget that it's needed
• we should not get build errors anymore that the BridgeClients are missing - that was happening quite often some time ago

Other than that, it removes the instantClients and continuousClients methods from the tests - again, it was something that was hiding all the configuration and people going through the tests were confused with the configuration of the clients - what is configured, how, what exactly is needed. Having the builders well defined in the tests will make the checks less confusing and more readable - and TBH, it's not that much stuff to maintain, given that we aren't touching the clients stuff that often.

Most of the refactoring is just changing from the old builders to new ones (or replacing the instant and continuous methods with the whole builders), the only main changes are in configuration of authentication, tracing, and Pod Security Profiles - for authentication we have a new class ClientsAuthentication, for tracing we have one method in OpenTelemetryST directly, same applies for adding Pod SecurityProfiles to clients' Jobs - that is also directly inside PodSecurityProfilesST.

NOTE: Currently I'm using 0.14.0-SNAPSHOT of Test-clients, as I was adding a lot of fixes to the main branch, which are not in the 0.13.0 release. I want to see if there are other changes required before releasing a new version and changing it here.

Checklist

• Make sure all tests pass
• Update documentation
• Try your changes from Pod inside your Kubernetes and OpenShift cluster, not just locally

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 75.07%. Comparing base (9949df4) to head (10eb10b).
⚠️ Report is 8 commits behind head on main.

Additional details and impacted files

@@             Coverage Diff              @@
##               main   #12640      +/-   ##
============================================
- Coverage     75.09%   75.07%   -0.02%     
- Complexity     6511     6512       +1     
============================================
  Files           376      376              
  Lines         25083    25083              
  Branches       3270     3271       +1     
============================================
- Hits          18835    18831       -4     
- Misses         4905     4908       +3     
- Partials       1343     1344       +1     

see 2 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[im-konge]

/gha run pipeline=regression,upgrade

[github-actions]

⏳ System test verification started: link

The following 10 job(s) will be executed:

• regression-brokers-and-security-amd64 (oracle-vm-8cpu-32gb-x86-64)
• regression-operators-amd64 (oracle-vm-8cpu-32gb-x86-64)
• regression-operands-amd64 (oracle-vm-8cpu-32gb-x86-64)
• regression-brokers-and-security-arm64 (oracle-vm-8cpu-32gb-arm64)
• regression-operators-arm64 (oracle-vm-8cpu-32gb-arm64)
• regression-operands-arm64 (oracle-vm-8cpu-32gb-arm64)
• upgrade-azp_kraft_upgrade-amd64 (oracle-vm-4cpu-16gb-x86-64)
• upgrade-azp_kafka_upgrade-amd64 (oracle-vm-4cpu-16gb-x86-64)
• upgrade-azp_kraft_upgrade-arm64 (oracle-vm-4cpu-16gb-arm64)
• upgrade-azp_kafka_upgrade-arm64 (oracle-vm-4cpu-16gb-arm64)

Tests will start after successful build completion.

[github-actions]

❌ System test verification failed: link

[im-konge]

/gha run pipeline=regression,upgrade

[github-actions]

⏳ System test verification started: link

The following 10 job(s) will be executed:

• regression-brokers-and-security-amd64 (oracle-vm-8cpu-32gb-x86-64)
• regression-operators-amd64 (oracle-vm-8cpu-32gb-x86-64)
• regression-operands-amd64 (oracle-vm-8cpu-32gb-x86-64)
• regression-brokers-and-security-arm64 (oracle-vm-8cpu-32gb-arm64)
• regression-operators-arm64 (oracle-vm-8cpu-32gb-arm64)
• regression-operands-arm64 (oracle-vm-8cpu-32gb-arm64)
• upgrade-azp_kraft_upgrade-amd64 (oracle-vm-4cpu-16gb-x86-64)
• upgrade-azp_kafka_upgrade-amd64 (oracle-vm-4cpu-16gb-x86-64)
• upgrade-azp_kraft_upgrade-arm64 (oracle-vm-4cpu-16gb-arm64)
• upgrade-azp_kafka_upgrade-arm64 (oracle-vm-4cpu-16gb-arm64)

Tests will start after successful build completion.

[github-actions]

🎉 System test verification passed: link

[Frawless]

Overall I am fine with that. I am just thinking about the naming. I know it will require also change on test-clients side, but maybe that's something we could discuss.

KafkaConsumerClient
KafkaProducerClient
KafkaProducerConsumer
KafkaAdminClient

Shouldn't it be KafkaProducerConsumerClient ? Or Just KafkaClients ? But it would mean we will need to add admin client into as well? Same for Http client. Or should we pick something like StrimziTestClient and have method to return producer, consumer, etc. ? Wdyt @see-quick @kornys

[im-konge]

Overall I am fine with that. I am just thinking about the naming. I know it will require also change on test-clients side, but maybe that's something we could discuss.

KafkaConsumerClient KafkaProducerClient KafkaProducerConsumer KafkaAdminClient

Shouldn't it be KafkaProducerConsumerClient ? Or Just KafkaClients ? But it would mean we will need to add admin client into as well? Same for Http client. Or should we pick something like StrimziTestClient and have method to return producer, consumer, etc. ? Wdyt @see-quick @kornys

TBH, I had PR in test-clients repo that added these builders, they looked fine.

I don't think that you need to have Admin and Streams clients in all the tests, that's why I created KafkaProducerConsumer, so you know what you will get and you don't need to configure all other things. Also you will not forget about configuring some stuff for Streams if you use it. Streams is used just in tracing tests. Admin client in few others, but mostly we are using Producer and Consumer.

I picked KafkaProducerClient because it would clash with KafkaProducer from Apache Kafka. And I'm not sure I like the StrimziTestClients, you are anyway taking it from the test-clients repo, the package has the io.strimzi group there and it would be I guess also a bit confusing what StrimziTestClients really mean.

But that's my opinion. And because it's public package, I would pick something that would make sense for long time and we will not change it again in few months, because it's named differently, so it confuses the user of the builders what it actually contains.

[see-quick]

Overall I am fine with that. I am just thinking about the naming. I know it will require also change on test-clients side, but maybe that's something we could discuss.

KafkaConsumerClient KafkaProducerClient KafkaProducerConsumer KafkaAdminClient

Shouldn't it be KafkaProducerConsumerClient ? Or Just KafkaClients ? But it would mean we will need to add admin client into as well? Same for Http client. Or should we pick something like StrimziTestClient and have method to return producer, consumer, etc. ? Wdyt @see-quick @kornys

You mean in consistency between those names that KafkaProducerConsumer doesn't have suffix -Client? If so I think then we should have KafkaProducerConsumerClients same for HTTP ones? Anyway I think I am also okay with having those two classes without that suffix as they bundle not single client but both.

Or Just KafkaClients ?

This would also imply that inside we have also streams, admin client ... not I think that is misleading as already pointed out by Lukas.

Or should we pick something like StrimziTestClient and have method to return producer, consumer, etc. ?

I think that's unnecessary. In the past, we had also clients defined this way as well (i.e., KafkaProducerJob, KafkaConsumerJob, KafkaAdminJob, KafkaProducerConsumerJob) as that ultimately tell you it produces Kubernetes Job and it tells you directly it is not like Apache Kafka clients. I don't know why we went different way (maybe I forgot something so sorry :D). But then we decide to go into KafkaClients and now we are going KafkaConsumerProducer ...

Anyway I am fine with current naming as it clearly states WHAT it is used in the tests and doesn't hide any important stuff. In terms of readability its clearly way forward.

[im-konge]

@Frawless @see-quick so what are we going to do with this? I'm for keeping it as it is, @see-quick looks like it as well. I'm not sure if there is someone else being +1 for renaming the builders.

[Frawless]

@Frawless @see-quick so what are we going to do with this? I'm for keeping it as it is, @see-quick looks like it as well. I'm not sure if there is someone else being +1 for renaming the builders.

Feel free to keep it as it is. I can live with it.