Skip to content

remove duplicate placeholder in sample config#1397

Merged
jpinsonneau merged 1 commit intonetobserv:mainfrom
jpinsonneau:config-fix
Apr 2, 2026
Merged

remove duplicate placeholder in sample config#1397
jpinsonneau merged 1 commit intonetobserv:mainfrom
jpinsonneau:config-fix

Conversation

@jpinsonneau
Copy link
Copy Markdown
Member

@jpinsonneau jpinsonneau commented Apr 2, 2026

Description

Fix sample

Dependencies

n/a

Checklist

  • Does the changes in PR need specific configuration or environment set up for testing?
    • if so please describe it in PR description.
  • I have added thorough unit tests for the change.
  • QE requirements (check 1 from the list):
    • Standard QE validation, with pre-merge tests unless stated otherwise.
    • Regression tests only (e.g. refactoring with no user-facing change).
    • No QE (e.g. trivial change with high reviewer's confidence, or per agreement with the QE team).

Summary by CodeRabbit

  • Chores
    • Removed duplicate configuration entry in the DNS flag response code filter to streamline settings.

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Apr 2, 2026

📝 Walkthrough

Walkthrough

A duplicate placeholder configuration line was removed from the sample YAML config file, leaving a single placeholder value for the DNS flag response code filter.

Changes

Cohort / File(s) Summary
Configuration
config/sample-config.yaml
Removed duplicate frontend.filters[id: dns_flag_response_code].placeholder entry, retaining one placeholder value ('E.g: NoError, NXDomain, NotAuth').

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~2 minutes

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 inconclusive)

Check name Status Explanation Resolution
Description check ❓ Inconclusive The description is too vague. 'Fix sample' provides no meaningful context about what was fixed or why. Expand the Description section to explain what issue was fixed and why the duplicate placeholder needed removal.
✅ Passed checks (2 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Title check ✅ Passed The title accurately summarizes the main change: removing a duplicate placeholder line from the sample config file.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands and usage tips.

@jpinsonneau jpinsonneau requested a review from jotak April 2, 2026 08:31
@jpinsonneau jpinsonneau added the no-qe This PR doesn't necessitate QE approval label Apr 2, 2026
@jpinsonneau jpinsonneau changed the title remove duplicate placeholder remove duplicate placeholder in sample config Apr 2, 2026
Copy link
Copy Markdown
Member

@jotak jotak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM
But I just ran the provided script to update this config, so is it that it's also wrong on the operator side?

@openshift-ci
Copy link
Copy Markdown

openshift-ci Bot commented Apr 2, 2026

[APPROVALNOTIFIER] This PR is APPROVED

Approval requirements bypassed by manually added approval.

This pull-request has been approved by:

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@jpinsonneau
Copy link
Copy Markdown
Member Author

LGTM But I just ran the provided script to update this config, so is it that it's also wrong on the operator side?

ok also created netobserv/netobserv-operator#2616
I took the liberty to approve it directly since it's the same changes as here

@jpinsonneau jpinsonneau merged commit e28231f into netobserv:main Apr 2, 2026
8 of 10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved lgtm no-qe This PR doesn't necessitate QE approval

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants