DOCS-3043: Add override mechanism for known-stale upstream Felix config doc text - #3019
Merged
Merged
Conversation
…ig doc text FlowLogsDynamicAggregationEnabled has always defaulted to false in Felix, but the doc comment feeding config-params.json said "Default is true." on every release branch. calico-private#12538 fixed the comment on master only, incidental to unrelated fluentd-removal work, and it was never backported. Add scripts/felix-config-overrides.json (known upstream doc-text bugs, keyed by NameConfigFile) and scripts/patch-felix-config-overrides.sh (jq-based, field-scoped patch that warns when an override becomes a no-op or stale), wired into update_felix_config() so every future sync patches it consistently. Apply it now to fix the currently-published files.
✅ Deploy Preview for calico-docs-preview-next ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
✅ Deploy Preview succeeded!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify project configuration. |
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The new override patch script needs small robustness fixes (argument/target validation and ensuring jq failures propagate correctly) before it’s safe to rely on during sync.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR introduces a small, reusable override mechanism to correct known-stale upstream Felix config documentation text during sync, preventing incorrect prose from being re-imported into config-params.json on every update.
Changes:
- Wire an override patch step into
scripts/update-felix-config.shimmediately before replacing the checked-inconfig-params.json. - Add
scripts/patch-felix-config-overrides.sh(jq-based, field/key-scoped patching + reporting) andscripts/felix-config-overrides.json(override definitions keyed byNameConfigFile). - Apply the corrected default text for
FlowLogsDynamicAggregationEnabledacross the currently published CE/CC versions affected.
File summaries
| File | Description |
|---|---|
| scripts/update-felix-config.sh | Runs the override patcher on the freshly downloaded JSON before moving it into place. |
| scripts/patch-felix-config-overrides.sh | Implements field/key-scoped override application and emits warnings/notices about override status. |
| scripts/felix-config-overrides.json | Declares the known-stale upstream doc-text override(s) keyed by NameConfigFile. |
| calico-enterprise_versioned_docs/version-3.24-2/_includes/components/FelixConfig/config-params.json | Fixes the incorrect “Default is true.” prose to “Default is false.” for the affected field. |
| calico-enterprise_versioned_docs/version-3.24-1/_includes/components/FelixConfig/config-params.json | Same doc-text correction for this published version. |
| calico-enterprise_versioned_docs/version-3.23-2/_includes/components/FelixConfig/config-params.json | Same doc-text correction for this published version. |
| calico-enterprise_versioned_docs/version-3.22-2/_includes/components/FelixConfig/config-params.json | Same doc-text correction for this published version. |
| calico-enterprise_versioned_docs/version-3.21-2/_includes/components/FelixConfig/config-params.json | Same doc-text correction for this published version. |
| calico-cloud/_includes/components/FelixConfig/config-params.json | Same doc-text correction for “next” (unversioned) Cloud docs. |
| calico-cloud_versioned_docs/version-23-2/_includes/components/FelixConfig/config-params.json | Same doc-text correction for the published Cloud version. |
Review details
Suppressed comments (1)
scripts/patch-felix-config-overrides.sh:51
- With
set -e, failures inside process substitution (done < <(jq ...)) don't reliably terminate the script, which can hide problems generating the report. Feeding the loop with a here-string from command substitution makes jq failures propagate correctly.
done < <(jq -r --slurpfile overrides "$OVERRIDES_FILE" '
$overrides[0] as $ovs
| [ .Groups[].Fields[] as $f
| $ovs[] as $ov
| select($ov.field == $f.NameConfigFile)
- Files reviewed: 10/10 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Addresses Copilot review feedback on PR 3019: a missing or non-JSON target previously surfaced as a raw jq error instead of a clear one.
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
The sync-time override mechanism and the versioned doc updates are straightforward and low-risk, with only a minor maintainability suggestion outstanding.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
scripts/patch-felix-config-overrides.sh:49
- The override reporting/apply logic only iterates over overrides that match a
NameConfigFilepresent in the target JSON. If an override’sfieldis renamed/removed upstream (or otherwise not present), it becomes silently ignored, which makes it easy to miss that an override is now obsolete or misconfigured. Add an explicit warning for override entries whosefielddoes not exist in the target JSON.
- Files reviewed: 10/10 changed files
- Comments generated: 0 new
- Review effort level: Lite
Two issues from an independent review of PR 3019: - The diagnostics jq ran inside a process substitution consumed by a while-read loop; under set -e a failure there was silently swallowed instead of aborting the script. - The diagnostics pass and the apply pass were two separately-written jq programs (one comparing against the pristine field, the other folding patches through a reduce accumulator). They agree today, but nothing enforced that as more overrides accumulate over time. Replaced both with a single jq invocation that computes each patch's status against the original document once, and applies exactly those decisions -- so the notices and the applied document share one source of truth and can't disagree, and the target file is only parsed once.
Comment on lines
+49
to
+52
| # --- Decide every patch's status against the ORIGINAL document, once, then apply exactly | ||
| # those decisions. The notices and the applied document come from the same "ops" list | ||
| # inside a single jq invocation, so they cannot disagree with each other, and the | ||
| # (multi-thousand-line) target file is only parsed once. --- |
Collaborator
Author
|
CC @hjiawei |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
FlowLogsDynamicAggregationEnabled has always defaulted to false in Felix, but the doc comment feeding config-params.json said "Default is true." on every release branch -- a table-vs-prose contradiction visible on the live reference page. tigera/calico-private#12538 fixed the comment on master only, incidental to unrelated fluentd-removal work, and it was never backported to any release branch.
This adds a small, reusable mechanism so update-felix-config.sh can patch known upstream doc-text bugs like this at sync time, instead of silently re-importing them on every run:
Also applies the fix now to every currently-published version that has it (CE 3.21-2 through 3.24-2, CC 23-2, and CC/CE "next").
Separately (DOCS-3043 doesn't cover this): the one-line comment fix needs to be cherry-picked from calico-private master into release-calient-v3.24-1, release-calient-v3.24, and other active branches, so this override eventually becomes unnecessary.
Jira: https://tigera.atlassian.net/browse/DOCS-3043
Preview (once it builds): https://deploy-preview-3019--tigera.netlify.app/calico-enterprise/latest/reference/resources/felixconfig#flowLogsDynamicAggregationEnabled