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

Add the broker classes consideration into the test cases #7660

Open
Leo6Leo opened this issue Feb 7, 2024 · 2 comments
Open

Add the broker classes consideration into the test cases #7660

Leo6Leo opened this issue Feb 7, 2024 · 2 comments
Labels
area/test-and-release Test infrastructure, tests or release kind/feature-request triage/accepted Issues which should be fixed (post-triage)

Comments

@Leo6Leo
Copy link
Member

Leo6Leo commented Feb 7, 2024

Problem
Adding more test cases to test out the broker classes configuration in the default broker class configuration.
The tests that get involved are unit tests.

Files involved:
pkg/apis/eventing/v1/broker_defaults_test.go

Persona:
Which persona is this feature for? Contributors

Exit Criteria
A measurable (binary) test that would indicate that the problem has been resolved.

Time Estimate (optional):
How many developer-days do you think this may take to resolve? 1

Additional context (optional)
#7631 (comment)

/triage accepted

@Leo6Leo Leo6Leo added kind/feature-request area/test-and-release Test infrastructure, tests or release labels Feb 7, 2024
@knative-prow knative-prow bot added the triage/accepted Issues which should be fixed (post-triage) label Feb 7, 2024
@NeerajNagure
Copy link

I would like to work on this.@Leo6Leo can I know which broker classes are you talking about in broker_defaults_test.go so that I would get an idea about writing those test cases?

@Leo6Leo
Copy link
Member Author

Leo6Leo commented Apr 12, 2024

@NeerajNagure Thanks for the help! I am providing some extra information here, and hopefully that can help you better understand the issue!

This issue is a expansion on this Pull Request: #7631. I was trying to write the feature that users can set the customized broker configuration for different broker classes.

Let's take this as an example:
We have cluster default broker class as MTChannelBasedBroker, and the configuration is for MTChannelBasedBroker.

Before this PR, if you hope to create a Kafka Broker without providing the configuration, knative will use the cluster default broker class configuration, which is MTChannelBasedBroker's configuration.

After this PR, you will have the chance to set multiple broker configuration template as default for different broker classes. Right now, not only we have cluster default broker class as MTChannelBasedBroker, and the configuration is for MTChannelBasedBroker, we have a new field called BrokerClasses, and this contains a value of Kafka Broker and it's config. So instead of using MTChannelBasedBroker's config, knative now knows to use Kafka Broker's config.

"no config, uses namespace broker config, cluster class": {

As you can see in this file above, it contains a lot of unit tests, but the tests for this new feature is missing. So we would need your help!

Learn more about that PR in this doc., and let me know if you have any questions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/test-and-release Test infrastructure, tests or release kind/feature-request triage/accepted Issues which should be fixed (post-triage)
Projects
None yet
Development

No branches or pull requests

2 participants