MON-4578: Add new fields to ThanosQuerierConfig#2835
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
|
Hello @danielmellado! Some important instructions when contributing to openshift/api: |
📝 WalkthroughWalkthroughAdded three optional fields to ThanosQuerierConfig: Suggested reviewers
🚥 Pre-merge checks | ✅ 11 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (11 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)Error: build linters: unable to load custom analyzer "kubeapilinter": tools/_output/bin/kube-api-linter.so, plugin: not implemented Comment |
openshift-ci
Bot
added
the
size/L
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 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 |
|
/test verify-deps |
danielmellado
force-pushed
the
mon_crd_thanosquerier_fields
branch
from
2d8d4d8 to
b6ab5b9
Compare
danielmellado
changed the title
|
@danielmellado: This pull request references MON-4578 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci-robot
added
the
jira/valid-reference
|
/retest-required |
danielmellado
force-pushed
the
mon_crd_thanosquerier_fields
branch
from
b6ab5b9 to
d1fcdca
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
config/v1alpha1/tests/clustermonitorings.config.openshift.io/ClusterMonitoringConfig.yaml (1)
1505-1543: ⚡ Quick winConsider adding a test for requestLogging with empty object.
The file includes tests for empty nested objects with required fields (e.g., lines 2280-2282 for
systemd.collect: {}and lines 2290-2292 fornetClass.collect: {}). SincerequestLoggingis a nested object with a requiredpolicyfield, consider adding a similar test case to verify thatrequestLogging: {}is properly rejected with an error indicating the missing required field.📋 Suggested test case
- name: Should reject ThanosQuerierConfig with requestLogging empty object initial: | apiVersion: config.openshift.io/v1alpha1 kind: ClusterMonitoring spec: thanosQuerierConfig: requestLogging: {} expectedError: "spec.thanosQuerierConfig.requestLogging: Invalid value"This test should be added after line 1543 (after the invalid policy test) to maintain consistency with the pattern used for similar nested structures elsewhere in the file.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@config/v1alpha1/tests/clustermonitorings.config.openshift.io/ClusterMonitoringConfig.yaml` around lines 1505 - 1543, Add a new test case that verifies an empty requestLogging object is rejected: create a test named like "Should reject ThanosQuerierConfig with requestLogging empty object" that sets spec.thanosQuerierConfig.requestLogging: {} in the initial YAML and expects an error indicating the invalid/missing required field (e.g., expectedError containing "spec.thanosQuerierConfig.requestLogging: Invalid value" or similar). Place this test immediately after the existing invalid-policy test so it follows the other requestLogging validations and mirrors the pattern used for other nested-required-field tests (see existing tests referencing requestLogging and policy).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@config/v1alpha1/types_cluster_monitoring.go`:
- Around line 2480-2482: Update the type comment for
ThanosQuerierRequestLoggingConfig to explicitly document the kubebuilder
validation marker `+kubebuilder:validation:MinProperties=1` by stating that the
struct requires at least one of its fields to be set; mention optionality of
fields (if applicable) and that this constraint enforces at least one property
present when the config is provided so readers and generated docs reflect the
rule for ThanosQuerierRequestLoggingConfig.
---
Nitpick comments:
In
`@config/v1alpha1/tests/clustermonitorings.config.openshift.io/ClusterMonitoringConfig.yaml`:
- Around line 1505-1543: Add a new test case that verifies an empty
requestLogging object is rejected: create a test named like "Should reject
ThanosQuerierConfig with requestLogging empty object" that sets
spec.thanosQuerierConfig.requestLogging: {} in the initial YAML and expects an
error indicating the invalid/missing required field (e.g., expectedError
containing "spec.thanosQuerierConfig.requestLogging: Invalid value" or similar).
Place this test immediately after the existing invalid-policy test so it follows
the other requestLogging validations and mirrors the pattern used for other
nested-required-field tests (see existing tests referencing requestLogging and
policy).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: c6ef1740-3a27-44b6-8e96-cf4176e3fd58
⛔ Files ignored due to path filters (5)
config/v1alpha1/zz_generated.crd-manifests/0000_10_config-operator_01_clustermonitorings.crd.yamlis excluded by!**/zz_generated.crd-manifests/*config/v1alpha1/zz_generated.deepcopy.gois excluded by!**/zz_generated*config/v1alpha1/zz_generated.featuregated-crd-manifests/clustermonitorings.config.openshift.io/ClusterMonitoringConfig.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**config/v1alpha1/zz_generated.swagger_doc_generated.gois excluded by!**/zz_generated*openapi/generated_openapi/zz_generated.openapi.gois excluded by!openapi/**,!**/zz_generated*
📒 Files selected for processing (3)
config/v1alpha1/tests/clustermonitorings.config.openshift.io/ClusterMonitoringConfig.yamlconfig/v1alpha1/types_cluster_monitoring.gopayload-manifests/crds/0000_10_config-operator_01_clustermonitorings.crd.yaml
| // ThanosQuerierRequestLoggingConfig configures request logging for Thanos Querier. | ||
| // +kubebuilder:validation:MinProperties=1 | ||
| type ThanosQuerierRequestLoggingConfig struct { |
There was a problem hiding this comment.
Document the MinProperties constraint explicitly.
ThanosQuerierRequestLoggingConfig has +kubebuilder:validation:MinProperties=1, but the type comment doesn’t explicitly state that at least one field must be set.
As per coding guidelines, "All kubebuilder validation markers must be documented in the field's comment, including field optionality, length constraints, value constraints, and advanced validation rules".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@config/v1alpha1/types_cluster_monitoring.go` around lines 2480 - 2482, Update
the type comment for ThanosQuerierRequestLoggingConfig to explicitly document
the kubebuilder validation marker `+kubebuilder:validation:MinProperties=1` by
stating that the struct requires at least one of its fields to be set; mention
optionality of fields (if applicable) and that this constraint enforces at least
one property present when the config is provided so readers and generated docs
reflect the rule for ThanosQuerierRequestLoggingConfig.
everettraven
left a comment
everettraven
left a comment
There was a problem hiding this comment.
Aside from one minor comment, this LGTM.
Port ThanosQuerierConfig fields from the CMO ConfigMap API to the ClusterMonitoring CRD. LogLevel reuses the existing LogLevel enum (Error, Warn, Info, Debug). EnableRequestLogging and EnableCORS use a new ThanosQuerierToggle string enum (Enabled, Disabled) following the Kubernetes API convention of avoiding *bool fields. Signed-off-by: Daniel Mellado <dmellado@fedoraproject.org>
danielmellado
force-pushed
the
mon_crd_thanosquerier_fields
branch
from
d1fcdca to
0ed3b01
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
config/v1alpha1/tests/clustermonitorings.config.openshift.io/ClusterMonitoringConfig.yaml (1)
1505-1543: ⚡ Quick winAdd a missing rejection case for
requestLogging: {}.This covers valid and invalid enum values, but not the required-field path where
policyis omitted. Adding that case protects therequestLogging.policyrequired contract.🧪 Proposed test addition
- name: Should reject ThanosQuerierConfig with invalid requestLogging policy initial: | apiVersion: config.openshift.io/v1alpha1 kind: ClusterMonitoring spec: thanosQuerierConfig: requestLogging: policy: "InvalidValue" expectedError: 'spec.thanosQuerierConfig.requestLogging.policy: Unsupported value: "InvalidValue": supported values: "AllRequests", "NoRequests"' + - name: Should reject ThanosQuerierConfig with requestLogging missing policy + initial: | + apiVersion: config.openshift.io/v1alpha1 + kind: ClusterMonitoring + spec: + thanosQuerierConfig: + requestLogging: {} + expectedError: 'spec.thanosQuerierConfig.requestLogging'🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@config/v1alpha1/tests/clustermonitorings.config.openshift.io/ClusterMonitoringConfig.yaml` around lines 1505 - 1543, The test suite for ClusterMonitoring is missing a rejection case when thanosQuerierConfig.requestLogging is provided but omits the required field requestLogging.policy; add a new test case (similar to the other entries) named something like "Should reject ThanosQuerierConfig with missing requestLogging.policy" that uses an initial manifest with spec.thanosQuerierConfig.requestLogging: {} and sets expectedError to indicate the required field is missing (e.g., 'spec.thanosQuerierConfig.requestLogging.policy: Required value: must be specified'); place this alongside the existing tests for requestLogging to ensure the required-field validation for requestLogging.policy is covered.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In
`@config/v1alpha1/tests/clustermonitorings.config.openshift.io/ClusterMonitoringConfig.yaml`:
- Around line 1505-1543: The test suite for ClusterMonitoring is missing a
rejection case when thanosQuerierConfig.requestLogging is provided but omits the
required field requestLogging.policy; add a new test case (similar to the other
entries) named something like "Should reject ThanosQuerierConfig with missing
requestLogging.policy" that uses an initial manifest with
spec.thanosQuerierConfig.requestLogging: {} and sets expectedError to indicate
the required field is missing (e.g.,
'spec.thanosQuerierConfig.requestLogging.policy: Required value: must be
specified'); place this alongside the existing tests for requestLogging to
ensure the required-field validation for requestLogging.policy is covered.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 3f44a2c7-5afb-4b1d-bd5d-0019b8d14a51
⛔ Files ignored due to path filters (5)
config/v1alpha1/zz_generated.crd-manifests/0000_10_config-operator_01_clustermonitorings.crd.yamlis excluded by!**/zz_generated.crd-manifests/*config/v1alpha1/zz_generated.deepcopy.gois excluded by!**/zz_generated*config/v1alpha1/zz_generated.featuregated-crd-manifests/clustermonitorings.config.openshift.io/ClusterMonitoringConfig.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**config/v1alpha1/zz_generated.swagger_doc_generated.gois excluded by!**/zz_generated*openapi/generated_openapi/zz_generated.openapi.gois excluded by!openapi/**,!**/zz_generated*
📒 Files selected for processing (3)
config/v1alpha1/tests/clustermonitorings.config.openshift.io/ClusterMonitoringConfig.yamlconfig/v1alpha1/types_cluster_monitoring.gopayload-manifests/crds/0000_10_config-operator_01_clustermonitorings.crd.yaml
|
/test verify |
|
Verify check looked to fail on an unrelated issue. If it fails for a similar reason, this PR may need to be rebased on the latest |
|
@danielmellado: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
5 participants
Port ThanosQuerierConfig fields from the CMO ConfigMap API to the
ClusterMonitoring CRD. LogLevel reuses the existing LogLevel enum
(Error, Warn, Info, Debug). EnableRequestLogging and EnableCORS use
a new ThanosQuerierToggle string enum (Enabled, Disabled) following
the Kubernetes API convention of avoiding *bool fields.
Signed-off-by: Daniel Mellado dmellado@fedoraproject.org