TRT-2600: Determine OS variant from cluster data#3449
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
|
@petr-muller: This pull request references TRT-2600 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 story 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
WalkthroughOS determination was moved from job-name substring parsing to explicit cluster-data parsing and synthetic cluster-data for snapshots. A new clusterDataOS type and parser were added; variant-calculation functions were unexported and now accept clusterDataOS; setOS became a clusterDataOS method with mapping, defaulting, backfill and mixed-resolution logic. Changes
Sequence Diagram(s)sequenceDiagram
participant Snapshot as "VariantSnapshot"
participant Loader as "OCPVariantLoader"
participant Parser as "cluster-data parser"
participant Resolver as "Variant resolver"
Snapshot->>Loader: Identify(jobName)
Loader->>Parser: parseClusterDataIfAvailable(jobName) / syntheticClusterDataOS(jobName)
Parser-->>Loader: clusterDataOS
Loader->>Resolver: calculateVariantsForJob(jobName, variantFile, clusterDataOS)
Resolver->>Resolver: setOS(clusterDataOS) / mapOSStreamToVariant() / resolve()
Resolver-->>Loader: variants
Loader-->>Snapshot: newVariants[job] = variants
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 13 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (13 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.11.4)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions Comment |
openshift-ci
bot
added
the
approved
|
@petr-muller: This pull request references TRT-2600 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 story 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. |
|
@petr-muller: This pull request references TRT-2600 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 story 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. |
![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)
pkg/variantregistry/ocp_test.go (1)
1915-2077: Cover the JSON parser path too.
TestSetOSbuildsclusterDataOSdirectly, but production now depends onparseClusterDataOS(...)before any of this logic runs. A tag/casing drift in the nestedospayload would silently fall back to the release-major default and this suite would still stay green. Add one fixture-based test that unmarshals representativecluster-data.jsonand asserts the parsed struct or finalVariantOS.Possible test to add
+func TestParseClusterDataOS(t *testing.T) { + raw := []byte(`{ + "os": { + "Default": "rhcos9", + "ControlPlaneMachineConfigPool": "rhcos10", + "WorkerMachineConfigPool": "rhcos10", + "Additional": ["rhcos10"] + } + }`) + + assert.Equal(t, clusterDataOS{ + Default: "rhcos9", + ControlPlane: "rhcos10", + Workers: "rhcos10", + Additional: []string{"rhcos10"}, + }, parseClusterDataOS(raw)) +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/variantregistry/ocp_test.go` around lines 1915 - 2077, Add a new table-driven test (next to TestSetOS) that exercises the JSON parsing path by loading one or more representative cluster-data.json fixtures, calling parseClusterDataOS(...) to produce a clusterDataOS, then passing that into setOS(...) (or calling the high-level code path that uses parseClusterDataOS) and asserting VariantOS equals the expected value; ensure the fixtures include a nested "os" payload with realistic casing/keys to catch tag/casing drift and reference parseClusterDataOS, clusterDataOS, setOS, VariantOS and TestSetOS in the test so it validates both unmarshalling and the downstream OS selection logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/variantregistry/ocp.go`:
- Around line 1317-1320: The comparison loop in resolve() incorrectly treats
empty OS values in os.Additional as real values and thus may return "mixed";
change the loop over os.Additional (where it currently checks `if a != cp {
return "mixed" }`) to skip empty entries (treat `a == ""` as missing) before
comparing to `cp`—i.e., continue when `a` is empty so only non-empty additional
pool OS values are compared to the control-plane/worker `cp`.
---
Nitpick comments:
In `@pkg/variantregistry/ocp_test.go`:
- Around line 1915-2077: Add a new table-driven test (next to TestSetOS) that
exercises the JSON parsing path by loading one or more representative
cluster-data.json fixtures, calling parseClusterDataOS(...) to produce a
clusterDataOS, then passing that into setOS(...) (or calling the high-level code
path that uses parseClusterDataOS) and asserting VariantOS equals the expected
value; ensure the fixtures include a nested "os" payload with realistic
casing/keys to catch tag/casing drift and reference parseClusterDataOS,
clusterDataOS, setOS, VariantOS and TestSetOS in the test so it validates both
unmarshalling and the downstream OS selection logic.
🪄 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: Pro Plus
Run ID: 94ea1352-4a64-4133-a99a-12aac28ae938
📒 Files selected for processing (4)
pkg/variantregistry/ocp.gopkg/variantregistry/ocp_test.gopkg/variantregistry/snapshot.gopkg/variantregistry/snapshot.yaml
|
@petr-muller: This pull request references TRT-2600 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 story 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. |
petr-muller
force-pushed
the
trt-2600-os-from-clusterdata
branch
2 times, most recently
from
564ce30 to
732e359
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
pkg/variantregistry/ocp.go (1)
1318-1323:⚠️ Potential issue | 🟡 MinorIgnore unresolved empty
AdditionalOS entries before returningmixed.If an
Additionalentry is empty andDefaultis also empty, this path still compares""tocpand can incorrectly classify asmixedinstead of “missing additional OS data”.Suggested fix
for _, a := range os.Additional { if a == "" { a = os.Default } + if a == "" { + continue + } if a != cp { return "mixed" } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/variantregistry/ocp.go` around lines 1318 - 1323, The loop over os.Additional currently substitutes os.Default into empty entries and then compares to cp, which incorrectly returns "mixed" when both are empty; change the logic in the loop handling os.Additional so that after applying the os.Default fallback you ignore (continue past) entries that are still empty instead of comparing them to cp, ensuring unresolved empty Additional entries do not trigger the "mixed" return. Use the existing identifiers os.Additional, os.Default, cp and the branch that currently returns "mixed" to locate and update the code.
🧹 Nitpick comments (1)
pkg/variantregistry/ocp.go (1)
453-461: Surface cluster-data OS parse errors to logs.Swallowing JSON errors here silently falls back OS classification, which makes bad
cluster-data.jsonhard to detect.Suggested refactor
-func parseClusterDataOS(data []byte) clusterDataOS { +func parseClusterDataOS(data []byte) (clusterDataOS, error) { var raw struct { OS clusterDataOS `json:"os"` } if err := json.Unmarshal(data, &raw); err != nil { - return clusterDataOS{} + return clusterDataOS{}, err } - return raw.OS + return raw.OS, nil }- osData = parseClusterDataOS(clusterDataBytes) + parsedOS, err := parseClusterDataOS(clusterDataBytes) + if err != nil { + jLog.WithError(err).Warn("unable to parse nested os data from cluster data file, proceeding with defaults") + } else { + osData = parsedOS + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/variantregistry/ocp.go` around lines 453 - 461, parseClusterDataOS currently swallows JSON unmarshal errors; update the parseClusterDataOS function so that when json.Unmarshal(data, &raw) returns an error it logs that error (including error.Error() and a short summary of data) before returning clusterDataOS{}; use the package's existing logger if available (or the standard log package if not) and keep the function signature/return type the same so callers of parseClusterDataOS and the clusterDataOS type don't change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@pkg/variantregistry/ocp.go`:
- Around line 1318-1323: The loop over os.Additional currently substitutes
os.Default into empty entries and then compares to cp, which incorrectly returns
"mixed" when both are empty; change the logic in the loop handling os.Additional
so that after applying the os.Default fallback you ignore (continue past)
entries that are still empty instead of comparing them to cp, ensuring
unresolved empty Additional entries do not trigger the "mixed" return. Use the
existing identifiers os.Additional, os.Default, cp and the branch that currently
returns "mixed" to locate and update the code.
---
Nitpick comments:
In `@pkg/variantregistry/ocp.go`:
- Around line 453-461: parseClusterDataOS currently swallows JSON unmarshal
errors; update the parseClusterDataOS function so that when json.Unmarshal(data,
&raw) returns an error it logs that error (including error.Error() and a short
summary of data) before returning clusterDataOS{}; use the package's existing
logger if available (or the standard log package if not) and keep the function
signature/return type the same so callers of parseClusterDataOS and the
clusterDataOS type don't change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 4b2bebb5-e1b5-490d-b5f9-5558e060acd3
📒 Files selected for processing (2)
pkg/variantregistry/ocp.gopkg/variantregistry/ocp_test.go
|
@petr-muller: This pull request references TRT-2600 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 story 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. |
petr-muller
force-pushed
the
trt-2600-os-from-clusterdata
branch
from
732e359 to
cd64388
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
pkg/variantregistry/ocp.go (2)
453-461: Consider logging JSON parse errors for debugging.
parseClusterDataOSsilently returns an empty struct when JSON unmarshaling fails. While this fallback behavior is reasonable for resilience, logging the error would help diagnose issues when cluster-data.json has unexpected formats.Optional: Add error logging
-func parseClusterDataOS(data []byte) clusterDataOS { +func parseClusterDataOS(jLog logrus.FieldLogger, data []byte) clusterDataOS { var raw struct { OS clusterDataOS `json:"os"` } if err := json.Unmarshal(data, &raw); err != nil { + if jLog != nil { + jLog.WithError(err).Debug("unable to parse OS section from cluster data") + } return clusterDataOS{} } return raw.OS }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/variantregistry/ocp.go` around lines 453 - 461, The function parseClusterDataOS swallows json.Unmarshal errors and returns an empty clusterDataOS; update parseClusterDataOS to capture the error from json.Unmarshal and log it before returning (e.g., call your package logger or standard log with context like "failed to unmarshal cluster-data.json in parseClusterDataOS" plus the error and optionally the raw data), keeping the existing fallback behavior of returning clusterDataOS{} when unmarshalling fails.
1313-1341: Add godoc forresolve()to clarify fallback and "mixed" semantics.The method implements a multi-level fallback chain and returns "mixed" under specific conditions. A brief godoc would help readers understand the behavior without tracing through the code.
Suggested godoc
+// resolve derives the OS variant from cluster data pools. +// It backfills empty ControlPlane and Workers from Default, maps stream names +// (e.g., "rhel-9") to variant names (e.g., "rhcos9"), and returns: +// - "" if the control plane OS is empty after backfill +// - "mixed" if control plane, workers, or additional pools disagree +// - the unified OS variant otherwise func (os clusterDataOS) resolve() string {As per coding guidelines: "When adding new functions, types, or fields, include a brief godoc if the name alone would not make the purpose obvious to someone unfamiliar with the feature."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/variantregistry/ocp.go` around lines 1313 - 1341, Add a brief godoc comment for the method resolve on the clusterDataOS type describing its multi-level fallback and "mixed" semantics: explain that it chooses ControlPlane or Default as the control plane variant, falls back Workers to Default if empty, maps streams via mapOSStreamToVariant, returns "" if control plane resolves to empty, returns "mixed" if control plane and workers or any Additional entries (falling back to Default when empty) map to different variants, otherwise returns the resolved variant; attach this comment immediately above the resolve() method declaration.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/variantregistry/ocp.go`:
- Around line 453-461: The function parseClusterDataOS swallows json.Unmarshal
errors and returns an empty clusterDataOS; update parseClusterDataOS to capture
the error from json.Unmarshal and log it before returning (e.g., call your
package logger or standard log with context like "failed to unmarshal
cluster-data.json in parseClusterDataOS" plus the error and optionally the raw
data), keeping the existing fallback behavior of returning clusterDataOS{} when
unmarshalling fails.
- Around line 1313-1341: Add a brief godoc comment for the method resolve on the
clusterDataOS type describing its multi-level fallback and "mixed" semantics:
explain that it chooses ControlPlane or Default as the control plane variant,
falls back Workers to Default if empty, maps streams via mapOSStreamToVariant,
returns "" if control plane resolves to empty, returns "mixed" if control plane
and workers or any Additional entries (falling back to Default when empty) map
to different variants, otherwise returns the resolved variant; attach this
comment immediately above the resolve() method declaration.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 1ceb48d9-2759-4255-8230-4dd1f5cee28f
📒 Files selected for processing (4)
pkg/variantregistry/ocp.gopkg/variantregistry/ocp_test.gopkg/variantregistry/snapshot.gopkg/variantregistry/snapshot.yaml
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/variantregistry/snapshot.go
- pkg/variantregistry/snapshot.yaml
|
@petr-muller: This pull request references TRT-2600 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 story 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. |
petr-muller
force-pushed
the
trt-2600-os-from-clusterdata
branch
from
cd64388 to
e1d1af1
Compare
|
@petr-muller: This pull request references TRT-2600 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 story 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. |
|
@petr-muller: This pull request references TRT-2600 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 story 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. |
|
@petr-muller: This pull request references TRT-2600 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 story 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. |
|
@petr-muller: This pull request references TRT-2600 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 story 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. |
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 `@pkg/variantregistry/ocp_test.go`:
- Around line 1928-1932: The test case named "rhcos9-10 job name takes
precedence over cluster data" currently omits clusterDataOS so it can't fail if
setOS incorrectly prefers cluster data; update this table entry to set
clusterDataOS to a conflicting value (e.g., "rhcos8-6" or any value !=
"rhcos9-10") so that calling setOS (the function under test) must choose the
jobName-derived OS "rhcos9-10" to satisfy expectedOS, thereby validating the
precedence behavior.
🪄 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: Pro Plus
Run ID: c84b1675-6f1c-438d-956d-4e0280656f6d
📒 Files selected for processing (3)
pkg/variantregistry/ocp.gopkg/variantregistry/ocp_test.gopkg/variantregistry/snapshot.go
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/variantregistry/snapshot.go
- pkg/variantregistry/ocp.go
petr-muller
force-pushed
the
trt-2600-os-from-clusterdata
branch
2 times, most recently
from
d699a28 to
fcfcac7
Compare
|
@coderabbitai resume |
✅ Actions performedReviews resumed. |
|
@petr-muller: This pull request references TRT-2600 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 story 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. |
petr-muller
force-pushed
the
trt-2600-os-from-clusterdata
branch
from
fcfcac7 to
3e22231
Compare
petr-muller
force-pushed
the
trt-2600-os-from-clusterdata
branch
from
3e22231 to
f8053b4
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/variantregistry/ocp.go (1)
445-461: Consider adding brief godoc forclusterDataOSandparseClusterDataOS.While the struct fields use self-explanatory names matching the cluster-data.json structure, a brief comment explaining the purpose (e.g., "holds OS image stream information extracted from cluster-data.json") would help readers unfamiliar with the feature. Similarly,
parseClusterDataOSsilently returns an empty struct on parse errors—a short comment noting this is intentional fallback behavior would clarify the design.As per coding guidelines: "When adding new functions, types, or fields, include a brief godoc if the name alone would not make the purpose obvious to someone unfamiliar with the feature."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/variantregistry/ocp.go` around lines 445 - 461, Add brief godoc comments for the clusterDataOS type and the parseClusterDataOS function: for clusterDataOS explain it "holds OS image stream information extracted from cluster-data.json (fields map to JSON keys Default, ControlPlaneMachineConfigPool, WorkerMachineConfigPool, Additional)"; for parseClusterDataOS explain it parses the JSON envelope (raw.os) and that it intentionally returns an empty clusterDataOS on unmarshal error as a safe fallback. Place these comments immediately above the type and function declarations (clusterDataOS and parseClusterDataOS) so the intent and fallback behavior are clear to readers.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/variantregistry/ocp.go`:
- Around line 445-461: Add brief godoc comments for the clusterDataOS type and
the parseClusterDataOS function: for clusterDataOS explain it "holds OS image
stream information extracted from cluster-data.json (fields map to JSON keys
Default, ControlPlaneMachineConfigPool, WorkerMachineConfigPool, Additional)";
for parseClusterDataOS explain it parses the JSON envelope (raw.os) and that it
intentionally returns an empty clusterDataOS on unmarshal error as a safe
fallback. Place these comments immediately above the type and function
declarations (clusterDataOS and parseClusterDataOS) so the intent and fallback
behavior are clear to readers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 0ac2250b-e008-4aa4-809b-85c73e6cebc9
📒 Files selected for processing (3)
pkg/variantregistry/ocp.gopkg/variantregistry/ocp_test.gopkg/variantregistry/snapshot.go
|
@petr-muller: This pull request references TRT-2600 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 story 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. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/variantregistry/ocp.go (1)
1289-1296: Both switch branches return the same value.Currently both
case "5"anddefaultreturn"rhcos9", making the switch effectively a no-op. If this is intentional scaffolding for future release differentiation, consider adding a brief comment explaining the intent. Otherwise, this could be simplified to a direct return.💡 Possible simplification if no future differentiation planned
func defaultOSForReleaseMajor(major string) string { //nolint:unparam - switch major { - case "5": - return "rhcos9" - default: - return "rhcos9" - } + // Future: may differentiate by major version + _ = major + return "rhcos9" }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/variantregistry/ocp.go` around lines 1289 - 1296, The switch in defaultOSForReleaseMajor always returns "rhcos9" (both case "5" and default), so either simplify by replacing the switch with a direct return of "rhcos9" in function defaultOSForReleaseMajor, or if this is intentional scaffolding, keep the switch but add a concise comment above defaultOSForReleaseMajor explaining it intentionally returns "rhcos9" for all majors and will be extended for future release-specific OS choices.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/variantregistry/ocp.go`:
- Around line 1289-1296: The switch in defaultOSForReleaseMajor always returns
"rhcos9" (both case "5" and default), so either simplify by replacing the switch
with a direct return of "rhcos9" in function defaultOSForReleaseMajor, or if
this is intentional scaffolding, keep the switch but add a concise comment above
defaultOSForReleaseMajor explaining it intentionally returns "rhcos9" for all
majors and will be extended for future release-specific OS choices.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 6cb5a6f0-0aac-493f-b5ef-168262a9ee19
📒 Files selected for processing (3)
pkg/variantregistry/ocp.gopkg/variantregistry/ocp_test.gopkg/variantregistry/snapshot.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/variantregistry/snapshot.go
The OS variant was previously derived solely from job name patterns (rhcos9, rhcos10, rhcos9-10). This changes setOS to use structured OS image stream data from cluster-data.json, with a multi-level backfill chain: release major version -> default -> control plane / workers. The only job-name pattern preserved is rhcos9-10 (mixed OS), which takes precedence over cluster data. The clusterDataOS struct is parsed from the nested "os" JSON object in cluster data and threaded through the variant calculation pipeline. When all node pools agree on the OS, that value becomes the variant; when they disagree, the variant is "mixed". Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
petr-muller
force-pushed
the
trt-2600-os-from-clusterdata
branch
from
f8053b4 to
b7ef95b
Compare
|
@petr-muller: This pull request references TRT-2600 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 story 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. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/variantregistry/ocp.go (1)
1289-1296: Consider adding a brief godoc explaining the rationale.The
//nolint:unparamsuggests awareness that all branches return the same value. A brief comment noting this is intentional for forward-compatibility (when RHCOS 10 becomes default for some major version) would clarify intent for future maintainers.📝 Suggested documentation
+// defaultOSForReleaseMajor returns the default OS variant for a release major version. +// Currently returns rhcos9 for all versions; the switch prepares for future differentiation. func defaultOSForReleaseMajor(major string) string { //nolint:unparam🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/variantregistry/ocp.go` around lines 1289 - 1296, Add a brief godoc comment above the function defaultOSForReleaseMajor explaining that it intentionally returns "rhcos9" for all current release majors (hence the //nolint:unparam) for forward-compatibility, and note the intended behavior when/if newer RHCOS versions (e.g., rhcos10) become the default for specific majors so future maintainers understand why all branches currently return the same value and where to update the function.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/variantregistry/ocp.go`:
- Around line 1289-1296: Add a brief godoc comment above the function
defaultOSForReleaseMajor explaining that it intentionally returns "rhcos9" for
all current release majors (hence the //nolint:unparam) for
forward-compatibility, and note the intended behavior when/if newer RHCOS
versions (e.g., rhcos10) become the default for specific majors so future
maintainers understand why all branches currently return the same value and
where to update the function.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: ddd3adfb-3919-445d-91af-472c353a2927
📒 Files selected for processing (3)
pkg/variantregistry/ocp.gopkg/variantregistry/ocp_test.gopkg/variantregistry/snapshot.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/variantregistry/snapshot.go
|
Scheduling required tests: |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: neisw, petr-muller The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
Scheduling required tests: |
|
@petr-muller: 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. |
3 participants
Reworks OS variant detection to derive the value from structured cluster data (the
osobject in cluster-data.json added in openshift/origin#30827) instead of job name patterns. The cluster data contains OS image stream names for the default, control plane, and worker machine config pools. These stream names (e.g.rhel-9.6,rhel-10.0,rhel-9-nvidia) are mapped to variant values (rhcos9,rhcos10) using a regex that extracts the major version.When all node pools agree on the OS, that value becomes the variant; when they disagree, the variant is
mixed. Empty pool values are backfilled from the default, and if the default is also empty, it is inferred from the release major version. The only job-name pattern preserved isrhcos9-10, which takes precedence over cluster data.For the variant snapshot (which has no access to real cluster data), jobs with
rhcos10in their name get synthetic cluster data so the snapshot accurately reflects their expected OS variant.🤖 Generated with Claude Code
Summary by CodeRabbit
Improvements
Tests