Skip to content

fix: RA2.3 README test command references missing 90-pod.yaml - #215

Open
andrewwhitecdw wants to merge 1 commit into
NVIDIA:mainfrom
andrewwhitecdw:codequality/readme-ra2-3-readme-test-command-references
Open

fix: RA2.3 README test command references missing 90-pod.yaml#215
andrewwhitecdw wants to merge 1 commit into
NVIDIA:mainfrom
andrewwhitecdw:codequality/readme-ra2-3-readme-test-command-references

Conversation

@andrewwhitecdw

@andrewwhitecdw andrewwhitecdw commented Aug 17, 2026

Copy link
Copy Markdown

This PR corrects the documentation in profiles/spectrum-x/README.md: RA2.3 README test command references missing 90-pod.yaml.

Changes

  • profiles/spectrum-x/README.md: RA2.3 README test command references missing 90-pod.yaml, and documents deriving the actual bounded DaemonSet name from the rendered manifest so long identifiers do not fail.
  • tests/spectrum-x/readme_test.sh: add a regression test that verifies the README no longer references the obsolete Pod manifest, references the generated DaemonSet manifest (with optional identifier suffix), and documents bounded-name lookup.

Details

--- a/profiles/spectrum-x/README.md
+++ b/profiles/spectrum-x/README.md
@@ -1,13 +1,18 @@
 ## Testing
 
-Deploy the test pod to verify the configuration:
+Deploy the example DaemonSet to verify the configuration:
 
 ```bash
-kubectl apply -f 90-pod.yaml
+kubectl apply -f ./output/network-operator/90-example-daemonset-<identifier>.yaml

-Check the pod has access to all rails:
+Wait for the DaemonSet to be ready, then run a command in one of its pods.
+Because Kubernetes object names are bounded to 63 characters, the DaemonSet
+name inside the manifest may differ from the filename identifier when the
+group identifier is long. Extract the actual name from the rendered manifest
+(replace <identifier> with the group identifier from your generated filename):

-kubectl exec -it spectrum-x-multirail-test-pod -- sh -c "ip addr show && rdma link"
+DS_NAME=$(sed -n 's/^  name: //p' ./output/network-operator/90-example-daemonset-<identifier>.yaml | head -n1)
+kubectl rollout status daemonset/$DS_NAME
+kubectl exec -it daemonset/$DS_NAME -- sh -c "ip addr show && rdma link"

## Tests
- `tests/spectrum-x/readme_test.sh`

```bash
$ ./tests/spectrum-x/readme_test.sh
OK: README references the expected Spectrum-X example DaemonSet manifest and documents bounded-name lookup

Contributor guidelines

Per this repo's CONTRIBUTING.md:

  • All commits are signed off (Signed-off-by trailer, DCO).

@greptile-apps

greptile-apps Bot commented Aug 17, 2026

Copy link
Copy Markdown

Greptile Summary

The PR corrects the Spectrum-X RA2.3 testing instructions and adds a regression check for the documented manifest workflow.

  • Points kubectl apply at the generated, identifier-suffixed DaemonSet manifest.
  • Derives the actual bounded DaemonSet name from the rendered manifest, supporting long identifiers.
  • Adds a shell test guarding against the obsolete Pod manifest reference and missing DaemonSet-name lookup.

Confidence Score: 5/5

The PR appears safe to merge.

No blocking failure remains.

Important Files Changed

Filename Overview
profiles/spectrum-x/README.md The revised commands use the generated manifest path and extract its bounded metadata name, resolving the previously reported path, name, and long-identifier failures.
tests/spectrum-x/readme_test.sh Adds focused static checks that reject the obsolete manifest reference and require the generated DaemonSet workflow.

Reviews (3): Last reviewed commit: "fix: RA2.3 README test command reference..." | Re-trigger Greptile

Comment thread profiles/spectrum-x/README.md Outdated
Comment thread profiles/spectrum-x/README.md Outdated
@andrewwhitecdw
andrewwhitecdw force-pushed the codequality/readme-ra2-3-readme-test-command-references branch from 2b12d97 to 91befb9 Compare August 17, 2026 21:48
@andrewwhitecdw

Copy link
Copy Markdown
Author

@greptile-apps thanks — updated the README to use the generated manifest path (./output/network-operator/90-example-daemonset-<identifier>.yaml) and the correct DaemonSet name (spectrum-x-test-<identifier>).

@greptile-apps

greptile-apps Bot commented Aug 17, 2026

Copy link
Copy Markdown

The README update looks good — the path and DaemonSet name are now accurate. However, there's a test alignment issue worth flagging.

The test script in the PR description checks for:

grep -qE '\b90-example-daemonset\.yaml\b' "$README"

But the README now contains 90-example-daemonset-<identifier>.yaml, not 90-example-daemonset.yaml. That \b word-boundary + exact .yaml suffix pattern won't match the new form — the test would fail on the updated README.

The check needs to be loosened to match the actual string, e.g.:

if ! grep -qE '\b90-example-daemonset' "$README"; then

Or more precisely (to avoid false positives while still allowing the identifier suffix):

if ! grep -qE '\b90-example-daemonset(-[^.]+)?\.yaml\b' "$README"; then

Similarly, the DaemonSet name check (if one exists) would need to match spectrum-x-test-<identifier> rather than the old spectrum-x-multirail-test. Could you confirm whether tests/spectrum-x/readme_test.sh has been updated alongside the README change, or is that still pending?

Comment thread profiles/spectrum-x/README.md Outdated
Update the RA2.3 Spectrum-X README testing section to apply the generated example DaemonSet manifest and derive the actual bounded DaemonSet name from the rendered file. This avoids rollout/exec failures when long group identifiers are truncated and hashed in the Kubernetes object name.

Add tests/spectrum-x/readme_test.sh to guard against regressions in the README testing instructions.

Signed-off-by: andrewwhitecdw <andrewwhitecdw@users.noreply.github.com>
@andrewwhitecdw
andrewwhitecdw force-pushed the codequality/readme-ra2-3-readme-test-command-references branch from 91befb9 to 5622f7b Compare August 17, 2026 21:58
@andrewwhitecdw

Copy link
Copy Markdown
Author

@greptile-apps thanks for the follow-up. I addressed both points:

  1. Long identifiers / bounded DaemonSet name: the README now instructs users to extract the actual DaemonSet name from the rendered manifest (DS_NAME=$(sed -n 's/^ name: //p' ./output/network-operator/90-example-daemonset-<identifier>.yaml | head -n1)) and use that variable for kubectl rollout status and kubectl exec. This works whether the identifier is short or long/truncated+hashed.

  2. Test alignment: I added tests/spectrum-x/readme_test.sh with the loosened \b90-example-daemonset(-[^.]+)?\.yaml\b pattern and a check that the README documents the bounded-name lookup. The test passes locally.

Pushed as commit 5622f7b.

@greptile-apps

greptile-apps Bot commented Aug 17, 2026

Copy link
Copy Markdown

Both points are addressed cleanly. A few observations:

README (sed command): The extraction uses sed -n 's/^ name: //p' to pull the DaemonSet name. This works if metadata.name is indented with exactly two spaces, which is standard YAML from kubectl/Helm-rendered manifests — looks fine for this use case.

Test script alignment: The three checks now match the README content:

  1. No 90-pod.yaml reference ✓
  2. References 90-example-daemonset(-<suffix>)?.yaml
  3. Documents DS_NAME= + 90-example-daemonset extraction ✓

One minor nit on the test: the DS_NAME grep pattern (DS_NAME=.*90-example-daemonset) is technically order-sensitive — it requires the variable name before the path on the same line, which happens to be true in the README. This is fine and intentional here.

Everything looks good. The changes are well-scoped, the test guards the right behaviors, and the README now handles both the naming and the bounded-name truncation case correctly. LGTM.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant