-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
CO should be more verbose when resource name is missing #4075
CO should be more verbose when resource name is missing #4075
Conversation
Signed-off-by: Stanislav Knot <sknot@redhat.com>
...mon/src/main/java/io/strimzi/operator/common/operator/resource/AbstractResourceOperator.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is good improvement general. But ... it still does not help much I think. So I wonder if to really improve the UX you should either:
- actually catch these exceptions on higher level and provide a proper error message saying where is the name missing
- Validate the name in the right places before calling the get / getAsync and raise proper error even before calling this.
Yeah, I was thinking about this too. But, does it mean having this check at milion places? 😮 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Just one nit.
cluster-operator/src/main/java/io/strimzi/operator/cluster/model/ListenersValidator.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. Just one more question ... should we add it to the ListenersValidatorTests as well? Sorry, I didn't realized that in the last review :-(.
Thanks for the test @sknot-rh |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple of nits, but apart from that LGTM.
private static void validateBrokerCertChainAndKey(Set<String> errors, GenericKafkaListener listener) { | ||
if (listener.getConfiguration().getBrokerCertChainAndKey().getSecretName() == null | ||
|| listener.getConfiguration().getBrokerCertChainAndKey().getSecretName().isEmpty()) { | ||
errors.add("listener '" + listener.getName() + "' cannot have empty secret name in the brokerCertChainAndKey"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
errors.add("listener '" + listener.getName() + "' cannot have empty secret name in the brokerCertChainAndKey"); | |
errors.add("listener '" + listener.getName() + "' cannot have an empty secret name in the brokerCertChainAndKey"); |
|
||
if (listener.getConfiguration().getBrokerCertChainAndKey().getKey() == null | ||
|| listener.getConfiguration().getBrokerCertChainAndKey().getKey().isEmpty()) { | ||
errors.add("listener '" + listener.getName() + "' cannot have empty key in the brokerCertChainAndKey"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
errors.add("listener '" + listener.getName() + "' cannot have empty key in the brokerCertChainAndKey"); | |
errors.add("listener '" + listener.getName() + "' cannot have an empty key in the brokerCertChainAndKey"); |
|
||
if (listener.getConfiguration().getBrokerCertChainAndKey().getCertificate() == null | ||
|| listener.getConfiguration().getBrokerCertChainAndKey().getCertificate().isEmpty()) { | ||
errors.add("listener '" + listener.getName() + "' cannot have empty certificate in the brokerCertChainAndKey"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
errors.add("listener '" + listener.getName() + "' cannot have empty certificate in the brokerCertChainAndKey"); | |
errors.add("listener '" + listener.getName() + "' cannot have an empty certificate in the brokerCertChainAndKey"); |
@@ -230,6 +230,9 @@ protected boolean wasChanged(T oldVersion, T newVersion) { | |||
* @return The resource, or null if it doesn't exist. | |||
*/ | |||
public T get(String namespace, String name) { | |||
if (name == null || name.isEmpty()) { | |||
throw new IllegalArgumentException(resourceKind + " with an empty name cannot be configured. Please provide a name."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should include the namespace in the message.
@@ -240,6 +243,9 @@ public T get(String namespace, String name) { | |||
* @return A Future for the result. | |||
*/ | |||
public Future<T> getAsync(String namespace, String name) { | |||
if (name == null || name.isEmpty()) { | |||
throw new IllegalArgumentException(resourceKind + " with an empty name cannot be configured. Please provide a name."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, can we include the namespace in the message?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM (after fixing latest Tom's comments)
* Connect default logging not expanded (strimzi#4057) Signed-off-by: Stanislav Knot <sknot@redhat.com> * Add documentation for CC CORS (strimzi#4030) * Add documentation for CC CORS Signed-off-by: Kyle Liberti <kliberti@redhat.com> * Capitalize CORS references Signed-off-by: Kyle Liberti <kliberti@redhat.com> * Update CHANGELOG.MD Signed-off-by: Kyle Liberti <kliberti@redhat.com> * Remove note irrelevant to Cruise Control config Signed-off-by: Kyle Liberti <kliberti@redhat.com> * Addressing comments Signed-off-by: Kyle Liberti <kliberti@redhat.com> * Addressing more comments Signed-off-by: Kyle Liberti <kliberti@redhat.com> * Address another typo Signed-off-by: Kyle Liberti <kliberti@redhat.com> * Add check for inter.broker.protocol.version and warning to status conditions (strimzi#4058) * Add check for inter.broker.protocol.version and warning to status conditions Signed-off-by: Jakub Scholz <www@scholzj.com> * Fix spotbugs Signed-off-by: Jakub Scholz <www@scholzj.com> * Fix the version matching Signed-off-by: Jakub Scholz <www@scholzj.com> * Add comment and rename the pattern to make its purpose more clear Signed-off-by: Jakub Scholz <www@scholzj.com> * Address some fixes to avoid tests to delete CO in STs (strimzi#4068) Signed-off-by: Jakub Stejskal <xstejs24@gmail.com> * Make it possible to roll Kafka or ZooKeeper pods individually (strimzi#4070) * Make it possible to roll Kafka or ZooKeeper pods individually Signed-off-by: Jakub Scholz <www@scholzj.com> * CHANGELOG.md Signed-off-by: Jakub Scholz <www@scholzj.com> * Remove commented out code Signed-off-by: Jakub Scholz <www@scholzj.com> * Avoid changing custom resource status because of HashSet ordering (strimzi#4069) Signed-off-by: Jakub Scholz <www@scholzj.com> * [systemtest] Tests for deploying Kafka and KafkaConnect without CRBs and enabled/disabled RackAware (strimzi#4045) * add tests for missing CRBs and some improvements to KafkaConnectResource and utils Signed-off-by: Lukas Kral <lukywill16@gmail.com> * fixup! add tests for missing CRBs and some improvements to KafkaConnectResource and utils Signed-off-by: Lukas Kral <lukywill16@gmail.com> * change name of ST, do some cleanup in KafkaConnectResource (addressing Sam's comments) Signed-off-by: Lukas Kral <lukywill16@gmail.com> * fixes for failing tests after my changes Signed-off-by: Lukas Kral <lukywill16@gmail.com> * fixup! fixes for failing tests after my changes Signed-off-by: Lukas Kral <lukywill16@gmail.com> * Do not use ownerReference in UO and TO bindings into a different namespace (strimzi#4080) Signed-off-by: Jakub Scholz <www@scholzj.com> * Add test for chages in strimzi#3987 (strimzi#4087) Signed-off-by: Tom Bentley <tbentley@redhat.com> * Remove owner referneces from ClusterRoleBindings (strimzi#4077) * Remove ownerReferneces from ClusterRoleBindings Signed-off-by: Jakub Scholz <www@scholzj.com> * Review comments Signed-off-by: Jakub Scholz <www@scholzj.com> * Review comments II Signed-off-by: Jakub Scholz <www@scholzj.com> * Fix log message search in STs Signed-off-by: Jakub Scholz <www@scholzj.com> * [DOC] new rack awareness image (strimzi#4074) Signed-off-by: prmellor <pmellor@redhat.com> * [DOC] Add the guide for running multiple Connect instances also to the deploying guide (strimzi#4079) * Add the guide for running multiple Connect instances also to the deploying guide Signed-off-by: Jakub Scholz <www@scholzj.com> * Fix comment Signed-off-by: Jakub Scholz <www@scholzj.com> * [systemtest] Tests for NetworkPolicy enhancements (strimzi#4085) * add tests for np enhancements, create ST for NPs etc. Signed-off-by: Lukas Kral <lukywill16@gmail.com> * Jakub's comment Signed-off-by: Lukas Kral <lukywill16@gmail.com> * Jakub's comment vol.2 Signed-off-by: Lukas Kral <lukywill16@gmail.com> * Add forbidden prefix exceptions to CC docs (strimzi#4095) Signed-off-by: Kyle Liberti <kliberti@redhat.com> * strimzi#4035: Patch Rancher Cattle annotations when reconciling services (strimzi#4076) * Patch Rancher Cattle annotations when reconciling services (strimzi#4035) Signed-off-by: Menno Luiten <menno.luiten@ah.nl> * Moved configuration of load balancer annotation to whitelist in Annotations (strimzi#4035) Signed-off-by: Menno Luiten <menno.luiten@ah.nl> * Replace regex with startsWith for better performance Signed-off-by: Menno Luiten <menno.luiten@ah.nl> * Fix checkstyles Signed-off-by: Jakub Scholz <www@scholzj.com> Co-authored-by: Jakub Scholz <www@scholzj.com> * CO should be more verbose when resource name is missing (strimzi#4075) * CO should be more verbose when resource name is missing Signed-off-by: Stanislav Knot <sknot@redhat.com> * comments Signed-off-by: Stanislav Knot <sknot@redhat.com> * comments Signed-off-by: Stanislav Knot <sknot@redhat.com> * priority Signed-off-by: Stanislav Knot <sknot@redhat.com> * comments Signed-off-by: Stanislav Knot <sknot@redhat.com> * test Signed-off-by: Stanislav Knot <sknot@redhat.com> * comments Signed-off-by: Stanislav Knot <sknot@redhat.com> * Move Question issues to Discussions (strimzi#4100) Signed-off-by: Jakub Scholz <www@scholzj.com> * Update Helm Chart index.yaml file with 0.20.1 release (strimzi#4106) Signed-off-by: Jakub Scholz <www@scholzj.com> * [DOC] Fix version field in the sample Kafka YAML (strimzi#4107) Signed-off-by: Jakub Scholz <www@scholzj.com> * feat: Update Jackson version to 2.11.0 (strimzi#4102) * feat: Update Jackson version to 2.11.0 Update Jackson version from 2.10.2 to 2.11.0 to resolve security vulnerability CVE-2020-25649 Signed-off-by: Salma Saeed <salma.saeed@ibm.com> * refactor: Change jackson to version 2.10.5.1 Signed-off-by: Salma Saeed <salma.saeed@ibm.com> * refactor: Update jackson-databind to 2.10.5.1 Update jackson-databind to 2.10.5.1 for security fix. Signed-off-by: Salma Saeed <salma.saeed@ibm.com> * Make fetching CMs async (strimzi#4084) * Make fetching CMs async Signed-off-by: Stanislav Knot <sknot@redhat.com> * comments Signed-off-by: Stanislav Knot <sknot@redhat.com> * avoid using composite fut Signed-off-by: Stanislav Knot <sknot@redhat.com> * refactor Signed-off-by: Stanislav Knot <sknot@redhat.com> * fix Signed-off-by: Stanislav Knot <sknot@redhat.com> * Replace oc cluster up with minikube for PR jenkins jobs (strimzi#4098) * Replace oc cluster up with minikube Signed-off-by: Jakub Stejskal <xstejs24@gmail.com> * fixup! Replace oc cluster up with minikube Signed-off-by: Jakub Stejskal <xstejs24@gmail.com> * fixup! fixup! Replace oc cluster up with minikube Signed-off-by: Jakub Stejskal <xstejs24@gmail.com> * fixup! fixup! fixup! Replace oc cluster up with minikube Signed-off-by: Jakub Stejskal <xstejs24@gmail.com> * fixup! fixup! fixup! fixup! Replace oc cluster up with minikube Signed-off-by: Jakub Stejskal <xstejs24@gmail.com> * Set RBAC for all kube versions Signed-off-by: Jakub Stejskal <xstejs24@gmail.com> * Allow network policy on minikube Signed-off-by: Jakub Stejskal <xstejs24@gmail.com> * Revert changes for NP Signed-off-by: Jakub Stejskal <xstejs24@gmail.com> * Set default version properly Signed-off-by: Jakub Stejskal <xstejs24@gmail.com> * Set default versiopn fo result reporting from PR job to github Signed-off-by: Jakub Stejskal <xstejs24@gmail.com> * Prometheus operator does not parse configuration (strimzi#4104) * Prometheus operator does not parse configuration Signed-off-by: Stanislav Knot <sknot@redhat.com> * comments Signed-off-by: Stanislav Knot <sknot@redhat.com> * [DOC] Updates to schema reference for Kafka config (strimzi#4063) * [DOC] Updates to schema reference Signed-off-by: prmellor <pmellor@redhat.com> * review edits PP Signed-off-by: prmellor <pmellor@redhat.com> * [DOC] Clarify Kafka Connect dependencies when configuring MM2 (strimzi#4078) * [DOC] Clairify Kafka Connect dependencies when configuring MM2 Signed-off-by: prmellor <pmellor@redhat.com> * review edits JS Signed-off-by: prmellor <pmellor@redhat.com> * ZookepeerUpgradeST -> KafkaUpgradeDowngradeST (strimzi#4125) Signed-off-by: Jakub Stejskal <xstejs24@gmail.com> * Reflect rename of upgrade tests in azp (strimzi#4127) Signed-off-by: Jakub Stejskal <xstejs24@gmail.com> * Disable PrometheusST to see if it fixes regression bundle VII (strimzi#4129) Signed-off-by: Jakub Scholz <www@scholzj.com> * Bump up jupiter version (strimzi#4126) * Bump up jupiter version Signed-off-by: Stanislav Knot <sknot@redhat.com> * to chce trochu verit, hosi Signed-off-by: Stanislav Knot <sknot@redhat.com> * revert vertx Signed-off-by: Stanislav Knot <sknot@redhat.com> * Remove few tests from acceptance tag (strimzi#4132) * Remove few tests from acceptance profile Signed-off-by: Jakub Stejskal <xstejs24@gmail.com> * Remove redundant cc test from acceptance Signed-off-by: Jakub Stejskal <xstejs24@gmail.com> * Remove the PrometheusST system test class (strimzi#4131) * Try to find out what broke the PrometheusST tests Signed-off-by: Jakub Scholz <www@scholzj.com> * Try newer version of the Prometheus Operator Signed-off-by: Jakub Scholz <www@scholzj.com> * Remove the PrometheusST class which does not work proeprly on the Minikube on Azure and hasanyway little value Signed-off-by: Jakub Scholz <www@scholzj.com> * Remove the remains of the -server JVM option (strimzi#4134) * Remove the remains of the -server JVM option Signed-off-by: Jakub Scholz <www@scholzj.com> * Fix system tests Signed-off-by: Jakub Scholz <www@scholzj.com> * [DOC] kafka config assembly update for Using Guide (strimzi#4064) * [DOC] New config procedure for Kafka Signed-off-by: prmellor <pmellor@redhat.com> * review edits DL Signed-off-by: prmellor <pmellor@redhat.com> * [DOC] link/reference updates for update Kafka config section (strimzi#4066) Signed-off-by: prmellor <pmellor@redhat.com> * [DOC] Minor edits - link fix and typos (strimzi#4130) * [DOC] Minor edits - link fix and typos Signed-off-by: prmellor <pmellor@redhat.com> * more typos Signed-off-by: prmellor <pmellor@redhat.com> * doc gen for updated java file Signed-off-by: prmellor <pmellor@redhat.com> * Add support for Kafka 2.7.0 (strimzi#4115) * Add support for Kafka 2.7.0-RC5 Signed-off-by: Jakub Scholz <www@scholzj.com> * Fix failing STs Signed-off-by: Jakub Scholz <www@scholzj.com> * Fix ZookeeperUpgrade tests Signed-off-by: Jakub Scholz <www@scholzj.com> * Fix imports Signed-off-by: Jakub Scholz <www@scholzj.com> * Fix some more STs Signed-off-by: Jakub Scholz <www@scholzj.com> * Move to final version Signed-off-by: Jakub Scholz <www@scholzj.com> * Use the right Scala version Signed-off-by: Jakub Scholz <www@scholzj.com> * Fix YQ version in the GitHub Actions / CodeQL workflow (strimzi#4145) * Fix YQ version in the GitHub Actions / CodeQL workflow Signed-off-by: Jakub Scholz <www@scholzj.com> * Another try to fix the version Signed-off-by: Jakub Scholz <www@scholzj.com> * Try to speed up the build with Maven cache Signed-off-by: Jakub Scholz <www@scholzj.com> * Update Helm2 to handle change chart repository addresses (strimzi#4146) Signed-off-by: Jakub Scholz <www@scholzj.com> * [DOC] Files removed from Kafka config section (strimzi#4065) Signed-off-by: prmellor <pmellor@redhat.com> * Add missing updates to the generated docu files with Kafka versions (strimzi#4142) Signed-off-by: Jakub Scholz <www@scholzj.com> * Include some more useful targets into make all command (strimzi#4144) Signed-off-by: Jakub Scholz <www@scholzj.com> * Add PDF build for our documentation (strimzi#4143) * Add PDF buold for our documentation Signed-off-by: Jakub Scholz <www@scholzj.com> * Add docu_pdfclean to docu_clean target Signed-off-by: Jakub Scholz <www@scholzj.com> Co-authored-by: Stanislav Knot <sknot@redhat.com> Co-authored-by: Kyle Liberti <kliberti@redhat.com> Co-authored-by: Jakub Scholz <www@scholzj.com> Co-authored-by: Jakub Stejskal <xstejs24@gmail.com> Co-authored-by: Lukáš Král <53821852+im-konge@users.noreply.github.com> Co-authored-by: Tom Bentley <tombentley@users.noreply.github.com> Co-authored-by: PaulRMellor <47596553+PaulRMellor@users.noreply.github.com> Co-authored-by: Menno Luiten <mluiten@artifix.net> Co-authored-by: salmasaeed1 <41479845+salmasaeed1@users.noreply.github.com>
Signed-off-by: Stanislav Knot sknot@redhat.com
Type of change
Description
When the resource name in the CR is not specified,
operation().inNamespace(namespace).withName(name)
returns an exceptionName must be provided.
(coming from the fabric8). In the large CR it can be tricky to find, what name is missing. This PR adds information, what resource does have an empty name specified.Output after these changes:
Fixes #4022
Checklist