OCPBUGS-60769: telco-ran: update ptp4lConfig reference to tolerate ordering#690
OCPBUGS-60769: telco-ran: update ptp4lConfig reference to tolerate ordering#690abraham2512 wants to merge 3 commits intoopenshift-kni:mainfrom
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: abraham2512 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 |
abraham2512
changed the title
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughConsolidated repeated per-interface Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
@abraham2512: This pull request references Jira Issue OCPBUGS-60769, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. 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. |
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@telco-ran/configuration/kube-compare-reference/ptp-operator/configuration/PtpConfigDualCardGmWpc.yaml`:
- Around line 117-118: The current masterPorts regex aggregate (masterPorts)
allows any alphanumeric interface names and thus drops the shared named captures
iface_timeTx1 and iface_timeTx2 used by spec.profile.0.ts2phcConf and
spec.profile.0.ptp4lConf (metadata.yaml uses inlineDiffFunc: capturegroups),
breaking the enforced name-consistency; update the masterPorts matcher to
preserve per-interface named capture groups (iface_timeTx1 and iface_timeTx2)
instead of a generic alnum capture and make the pattern order-tolerant (like the
multi-card example PtpConfigThreeCardGmWpc.yaml) so ptp4lConf interface names
remain identical to those captured from ts2phcConf while allowing fields to
appear in any order.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9a7dfc71-d669-4800-8a3f-7864512e78c4
📒 Files selected for processing (1)
telco-ran/configuration/kube-compare-reference/ptp-operator/configuration/PtpConfigDualCardGmWpc.yaml
| (?<masterPorts>((\[[[:alnum:]]+\] | ||
| masterOnly 1| *#.*| *)(\n|$))+) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n -C2 'pathToKey: spec\.profile\.0\.(ptp4lConf|ts2phcConf)|iface_timeTx1|iface_timeTx2|masterPorts' \
telco-ran/configuration/kube-compare-referenceRepository: openshift-kni/telco-reference
Length of output: 36069
Keep the shared iface_timeTx1 / iface_timeTx2 capture names in this matcher.
Both spec.profile.0.ts2phcConf and spec.profile.0.ptp4lConf are configured with inlineDiffFunc: capturegroups in metadata.yaml (lines 288-291). Named capture groups are shared across these fields, and repeated names enforce consistency. The masterPorts aggregate capture on lines 117-118 accepts any alphanumeric interface names, breaking the requirement that ptp4lConf interface names must match those captured from ts2phcConf (iface_timeTx1 and iface_timeTx2). Preserve the per-interface named captures while making the block order-tolerant, consistent with other multi-card GM configurations like PtpConfigThreeCardGmWpc.yaml.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@telco-ran/configuration/kube-compare-reference/ptp-operator/configuration/PtpConfigDualCardGmWpc.yaml`
around lines 117 - 118, The current masterPorts regex aggregate (masterPorts)
allows any alphanumeric interface names and thus drops the shared named captures
iface_timeTx1 and iface_timeTx2 used by spec.profile.0.ts2phcConf and
spec.profile.0.ptp4lConf (metadata.yaml uses inlineDiffFunc: capturegroups),
breaking the enforced name-consistency; update the masterPorts matcher to
preserve per-interface named capture groups (iface_timeTx1 and iface_timeTx2)
instead of a generic alnum capture and make the pattern order-tolerant (like the
multi-card example PtpConfigThreeCardGmWpc.yaml) so ptp4lConf interface names
remain identical to those captured from ts2phcConf while allowing fields to
appear in any order.
There was a problem hiding this comment.
An interesting observation, but not as important as it seems. It's true that we did have an extra layer of validation with the named-capture groups in the ptp4lConf, but the flexibility of allowing different numbers and different ordering of this section outweights that minor benefit.
There was a problem hiding this comment.
@lack, understood — the added flexibility in tolerating arbitrary ordering and varying numbers of masterOnly 1 port blocks is a deliberate trade-off over the stricter per-interface name validation. Thanks for the context!
📝 Learnings were identified and not saved (knowledge base disabled). Enable
There was a problem hiding this comment.
♻️ Duplicate comments (1)
telco-ran/configuration/kube-compare-reference/ptp-operator/configuration/PtpConfigDualCardGmWpc.yaml (1)
117-118:⚠️ Potential issue | 🟠 MajorDon't make the
masterOnlypreamble fully generic.Line 117-Line 118 now match any alphanumeric
masterOnly 1block, soptp4lConfno longer has to use the sameiface_timeTx1/iface_timeTx2interfaces captured byphc2sysOpts,ts2phcConf, and thepinsmap. That weakens this reference from “the expected dual-card ports in any order” to “any ports in any order,” which is broader than the PR objective. Keep the order-tolerant structure, but preserve the explicitiface_timeTx*named captures in the matcher.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@telco-ran/configuration/kube-compare-reference/ptp-operator/configuration/PtpConfigDualCardGmWpc.yaml` around lines 117 - 118, The masterOnly preamble regex was made too generic and loses the explicit iface_timeTx1/iface_timeTx2 captures used elsewhere (ptp4lConf, phc2sysOpts, ts2phcConf, pins); update the (?<masterPorts>...) group so it still allows multiple lines in any order but explicitly captures iface_timeTx1 and iface_timeTx2 (use the same named capture tokens iface_timeTx1 and iface_timeTx2 inside the masterOnly block instead of a generic [[:alnum:]]+), ensuring those named captures are present and consistent with phc2sysOpts, ts2phcConf and pins while preserving order-tolerance and comments handling.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In
`@telco-ran/configuration/kube-compare-reference/ptp-operator/configuration/PtpConfigDualCardGmWpc.yaml`:
- Around line 117-118: The masterOnly preamble regex was made too generic and
loses the explicit iface_timeTx1/iface_timeTx2 captures used elsewhere
(ptp4lConf, phc2sysOpts, ts2phcConf, pins); update the (?<masterPorts>...) group
so it still allows multiple lines in any order but explicitly captures
iface_timeTx1 and iface_timeTx2 (use the same named capture tokens iface_timeTx1
and iface_timeTx2 inside the masterOnly block instead of a generic
[[:alnum:]]+), ensuring those named captures are present and consistent with
phc2sysOpts, ts2phcConf and pins while preserving order-tolerance and comments
handling.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d2e4050f-b36f-49b2-b24a-4fe41ac19ac0
📒 Files selected for processing (2)
telco-ran/configuration/kube-compare-reference/hack/default_value.yamltelco-ran/configuration/kube-compare-reference/ptp-operator/configuration/PtpConfigDualCardGmWpc.yaml
abraham2512
force-pushed
the
ptp4lConf-update
branch
from
8f34abe to
9704780
Compare
| [(?<iface_timeTx2_2>[[:alnum:]]+)] | ||
| masterOnly 1 | ||
| [(?<iface_timeTx2_3>[[:alnum:]]+)] | ||
| masterOnly 1 |
There was a problem hiding this comment.
I think this could be a bit clearer now; Maybe since the capturegroups aren't in use any more, we could go with something a bit more human-readable:
# Include one section for each TimeTransmitter port on the WPC cards:
[ens1f0]
masterOnly 1
[ens1f1]
masterOnly 1
[ens2f0]
masterOnly 1
[ens2f1]
masterOnly 1
# ...
There was a problem hiding this comment.
Updated the reference with these interface names
abraham2512
force-pushed
the
ptp4lConf-update
branch
2 times, most recently
from
bcc1f87 to
acc9006
Compare
abraham2512
force-pushed
the
ptp4lConf-update
branch
from
acc9006 to
f2ad047
Compare
|
/cc @lack |
| captureGroup_defaults: | ||
| priority2: 128 | ||
| domainNumber: 24 | ||
| iface_timeRx: ens1f0 |
There was a problem hiding this comment.
Maybe use "$iface_timeRx" as the example, since which interface a customer will choose to be the time-receiver will be variable?
| captureGroup_defaults: | ||
| priority2: 128 | ||
| domainNumber: 24 | ||
| iface_timeRx: ens1f0 |
There was a problem hiding this comment.
3 participants
This PR updates the PtpConfigDualCardGmWpc reference CR to tolerate different ordering in ptp4lConf