-
Notifications
You must be signed in to change notification settings - Fork 70
🐛 Fix an unclear validation error when configuring watchNamespace on an operator restricted to AllNamespaces mode. #2347
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
Conversation
✅ Deploy Preview for olmv1 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
b5944d5 to
90ae0c1
Compare
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.
Pull Request Overview
This PR improves error messaging when users attempt to configure watchNamespace on operators that only support AllNamespaces install mode. The error message has been enhanced from a generic "unknown field" message to a more descriptive explanation indicating that only AllNamespaces install mode is supported.
Key Changes:
- Enhanced error message formatting in config validation to provide context-specific feedback for
watchNamespacefield rejection - Updated test expectations to reflect the new error message
- Removed MultiNamespace-related test cases, as this install mode is not supported by the validation system
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| internal/operator-controller/config/config.go | Added special case handling in error formatting to provide descriptive message when watchNamespace is rejected |
| internal/operator-controller/config/config_test.go | Updated test expectations for new error message and removed unsupported MultiNamespace test cases |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2347 +/- ##
==========================================
+ Coverage 74.23% 74.29% +0.05%
==========================================
Files 91 91
Lines 7239 7255 +16
==========================================
+ Hits 5374 5390 +16
Misses 1433 1433
Partials 432 432
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
90ae0c1 to
8eb47f8
Compare
8eb47f8 to
cc5d7fb
Compare
cc5d7fb to
d738410
Compare
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.
Pull Request Overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
d738410 to
0b567de
Compare
pedjak
left a comment
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
|
not sure if this qualifies as a bug fix, perhaps is just a 🌱 ? |
|
Hi @pedjak, Thank you for review 🥇
This is a bug. Before: The message shown to the end user was unclear and unhelpful. When to use 🌱 Use the 🌱 label ONLY when the change has zero impact on end users. This includes:
In other words, 🌱 is reserved strictly for internal, non-functional updates such as |
rashmigottipati
left a comment
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
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: pedjak, rashmigottipati The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
…mespace on an operator restricted to AllNamespaces mode.
0b567de to
f85c707
Compare
|
New changes are detected. LGTM label has been removed. |
|
/hold cancel @perdasilva @rashmigottipati @tmshort Sorry about my slip-up earlier. Everything is all set now! Thanks for the support |
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.
Pull Request Overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
The headline and PR/commit summary do not patch what the PR now does, which is to add a new install mode. |
|
@perdasilva can correct me if I am wrong, but my understanding of the overall intended progression of the registry+v1 configuration schema is that we will:
With that in mind (and if that is still true), I don't think we should be spending cycles implementing and reviewing the current validation implementation. The focus (I think) is on the order of operations above. |
|
Dropped a comment in the bug as well to try to explain our stance: https://issues.redhat.com/browse/OCPBUGS-63611?focusedId=28504231&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-28504231 Design decision doc also mentions this descoping aspect of OLMv1 near the very top: https://operator-framework.github.io/operator-controller/project/olmv1_design_decisions/#watched-namespaces-cannot-be-configured-in-a-first-class-api |
|
Hi @joelanford Regards your comment: #2347 (comment)
Yes, that is ALL true but we either have the option to create a customize validations and messages if we need to do so. You can check the design in: #2316 But I completely agree that we can’t create a custom message or behavior for every single flag and option, as that would lead to infinite maintenance cycles. I appreciate your comment in the JIRA, and I’m closing this one as won’t do. |
Summary of Changes
When a ClusterExtension with
watchNamespaceconfig is applied to an AllNamespaces-only, the error message has been improved to be more descriptive.Before vs After
Scenario: User tries to configure watchNamespace on AllNamespaces-only operator
BEFORE:
NOW:
Motivation
https://issues.redhat.com//browse/OCPBUGS-63611