Skip to content

Update verify-feature-promotion HTML output to sort entries and fix overflow#2793

Open
JoelSpeed wants to merge 2 commits intoopenshift:masterfrom
JoelSpeed:fix-verify-feature-output
Open

Update verify-feature-promotion HTML output to sort entries and fix overflow#2793
JoelSpeed wants to merge 2 commits intoopenshift:masterfrom
JoelSpeed:fix-verify-feature-output

Conversation

@JoelSpeed
Copy link
Copy Markdown
Contributor

@JoelSpeed JoelSpeed commented Apr 2, 2026

Fixes the ordering of the variants so that it's stable between runs, also adds some padding to the bottom of the content as currently it's being cropped

Proof of newest output in https://prow.ci.openshift.org/view/gs/test-platform-results/pr-logs/pull/openshift_api/2793/pull-ci-openshift-api-master-verify-feature-promotion/2039838674643849216

@openshift-ci-robot
Copy link
Copy Markdown

Pipeline controller notification
This repo is configured to use the pipeline controller. Second-stage tests will be triggered either automatically or after lgtm label is added, depending on the repository configuration. The pipeline controller will automatically detect which contexts are required and will utilize /test Prow commands to trigger the second stage.

For optional jobs, comment /test ? to see a list of all defined jobs. To trigger manually all jobs from second stage use /pipeline required command.

This repository is configured in: LGTM mode

@openshift-ci
Copy link
Copy Markdown
Contributor

openshift-ci bot commented Apr 2, 2026

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 2, 2026
@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Apr 2, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository YAML (base), Organization UI (inherited)

Review profile: CHILL

Plan: Pro

Run ID: 5c8b7fed-dfd4-4b2e-8b1a-f8c9ef81a18a

📥 Commits

Reviewing files that changed from the base of the PR and between ad9eb11 and 7480ccd.

📒 Files selected for processing (2)
  • tools/codegen/cmd/featuregate-test-analyzer.go
  • tools/codegen/pkg/utils/html.go

📝 Walkthrough

Walkthrough

The pull request modifies how HTML feature gate table variants are ordered and styled. The featuregate-test-analyzer command now wraps unsorted variant lists with OrderedJobVariants(...) and uses an updated Less comparison method that explicitly handles NetworkStack, OS, and JobTiers ordering, replacing the previous sort.SliceStable approach with int-keyed network stack mapping. Network-stack ordering now uses a string-keyed rank map instead. Additionally, the HTML template in utils/html.go receives a CSS styling update, adding padding-bottom: 20px to the .container-fluid class. No exported or public function signatures were altered.

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

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: build linters: unable to load custom analyzer "kubeapilinter": tools/_output/bin/kube-api-linter.so, plugin: not implemented
The command is terminated due to an error: build linters: unable to load custom analyzer "kubeapilinter": tools/_output/bin/kube-api-linter.so, plugin: not implemented


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

@openshift-ci
Copy link
Copy Markdown
Contributor

openshift-ci bot commented Apr 2, 2026

Hello @JoelSpeed! Some important instructions when contributing to openshift/api:
API design plays an important part in the user experience of OpenShift and as such API PRs are subject to a high level of scrutiny to ensure they follow our best practices. If you haven't already done so, please review the OpenShift API Conventions and ensure that your proposed changes are compliant. Following these conventions will help expedite the api review process for your PR.

@openshift-ci openshift-ci bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Apr 2, 2026
@openshift-ci
Copy link
Copy Markdown
Contributor

openshift-ci bot commented Apr 2, 2026

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign deads2k for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found 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

@JoelSpeed
Copy link
Copy Markdown
Contributor Author

/testwith openshift/api/master/verify-feature-promotion #2772

@JoelSpeed
Copy link
Copy Markdown
Contributor Author

/test verify-feature-promotion

@qodo-code-review
Copy link
Copy Markdown

qodo-code-review bot commented Apr 2, 2026

PR-Agent: could not fine a component named verify-feature-promotion in a supported language in this PR.

@JoelSpeed
Copy link
Copy Markdown
Contributor Author

/test verify-feature-promotion

@qodo-code-review
Copy link
Copy Markdown

qodo-code-review bot commented Apr 2, 2026

PR-Agent: could not fine a component named verify-feature-promotion in a supported language in this PR.

@JoelSpeed JoelSpeed force-pushed the fix-verify-feature-output branch from 0c8a526 to c0a7c47 Compare April 2, 2026 22:53
@JoelSpeed
Copy link
Copy Markdown
Contributor Author

/test verify-feature-promotion

@qodo-code-review
Copy link
Copy Markdown

qodo-code-review bot commented Apr 2, 2026

PR-Agent: could not fine a component named verify-feature-promotion in a supported language in this PR.

@JoelSpeed JoelSpeed force-pushed the fix-verify-feature-output branch from c0a7c47 to 7480ccd Compare April 2, 2026 23:00
@JoelSpeed JoelSpeed marked this pull request as ready for review April 2, 2026 23:01
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 2, 2026
@openshift-ci openshift-ci bot requested review from deads2k and everettraven April 2, 2026 23:02
@qodo-code-review
Copy link
Copy Markdown

Review Summary by Qodo

Stabilize HTML output sorting and fix table overflow

✨ Enhancement 🐞 Bug fix

Grey Divider

Walkthroughs

Description
• Refactor job variant sorting to use centralized OrderedJobVariants method
• Add comprehensive sorting criteria including NetworkStack, OS, and JobTiers
• Add CSS padding to prevent table content from being cut off
• Replace inline sorting logic with reusable sort implementation
Diagram
flowchart LR
  A["buildHTMLFeatureGate<br/>function"] -->|"uses"| B["OrderedJobVariants<br/>sort method"]
  B -->|"sorts by"| C["Topology, Architecture,<br/>NetworkStack, OS, JobTiers"]
  D["HTML template"] -->|"adds CSS"| E["container-fluid<br/>padding-bottom"]
  A -->|"generates"| D
Loading

Grey Divider

File Changes

1. tools/codegen/cmd/featuregate-test-analyzer.go ✨ Enhancement +32/-13

Centralize and extend job variant sorting logic

• Replace inline NetworkStack sorting with centralized OrderedJobVariants method
• Extend OrderedJobVariants.Less() to include NetworkStack, OS, and JobTiers comparison logic
• Remove hardcoded network stack ordering map from buildHTMLFeatureGate function
• Fix minor formatting inconsistency in TestingResults struct field alignment

tools/codegen/cmd/featuregate-test-analyzer.go


2. tools/codegen/pkg/utils/html.go 🐞 Bug fix +1/-0

Add CSS padding to prevent content cutoff

• Add CSS padding-bottom to .container-fluid class to prevent table overflow
• Ensure content is not cropped at bottom of HTML output

tools/codegen/pkg/utils/html.go


Grey Divider

Qodo Logo

@qodo-code-review
Copy link
Copy Markdown

qodo-code-review bot commented Apr 2, 2026

Code Review by Qodo

🐞 Bugs (2) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX Issues (0)

Grey Divider


Action required

1. Optional not in sort 🐞 Bug ≡ Correctness
Description
OrderedJobVariants.Less() never compares JobVariant.Optional, so two distinct variants that
differ only by Optional are treated as equal and sort.Sort may emit them in a non-deterministic
order when the input comes from UnsortedList(). This can break the PR goal of stable HTML/Markdown
column ordering if optional/required variants share the same other fields.
Code

tools/codegen/cmd/featuregate-test-analyzer.go[R655-667]

+	if strings.Compare(a[i].OS, a[j].OS) < 0 {
+		return true
+	} else if strings.Compare(a[i].OS, a[j].OS) > 0 {
+		return false
+	}
+
+	if strings.Compare(a[i].JobTiers, a[j].JobTiers) < 0 {
+		return true
+	} else if strings.Compare(a[i].JobTiers, a[j].JobTiers) > 0 {
+		return false
+	}
+
	return false
Evidence
buildHTMLFeatureGateData() builds jobVariants from sets.KeySet(testingResults).UnsortedList()
and then sorts it with sort.Sort, so the comparator must fully order all fields that can differ to
guarantee deterministic output. JobVariant includes an Optional field, but
OrderedJobVariants.Less() compares Topology/Cloud/Architecture/NetworkStack/OS/JobTiers and then
returns false (tie) without considering Optional, allowing distinct values to compare equal and
leaving their relative order dependent on the non-ordered input slice.

tools/codegen/cmd/featuregate-test-analyzer.go[263-268]
tools/codegen/cmd/featuregate-test-analyzer.go[607-615]
tools/codegen/cmd/featuregate-test-analyzer.go[621-668]
tools/codegen/cmd/featuregate-test-analyzer.go[417-421]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`OrderedJobVariants.Less()` does not include `JobVariant.Optional` in its comparison chain. That means two distinct variants that only differ by `Optional` compare equal, and since inputs come from `UnsortedList()` the final ordering can still vary across runs.

### Issue Context
Both HTML and Markdown outputs sort variants derived from a set/map key list.

### Fix Focus Areas
- tools/codegen/cmd/featuregate-test-analyzer.go[621-668]

### Suggested fix
Add a final comparison for `Optional` (and keep it last so it only affects ties), e.g.:
- if `a[i].Optional != a[j].Optional`, return `!a[i].Optional && a[j].Optional` (non-optional first)

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools



Remediation recommended

2. Unknown NetworkStack ranks lowest 🐞 Bug ⚙ Maintainability
Description
The new NetworkStack ordering uses a map lookup without handling unknown keys, so an unexpected/typo
NetworkStack value gets the zero value "" rank and sorts before the intended ""/ipv4/ipv6/dual
order. This can silently put bad/unknown values first and produce surprising column ordering.
Code

tools/codegen/cmd/featuregate-test-analyzer.go[R640-653]

+	// Map these to an ordered list of strings so that we can define the order
+	// rather than them being alphabetical.
+	var networkStackOrder = map[string]string{
+		"":     "0",
+		"ipv4": "1",
+		"ipv6": "2",
+		"dual": "3",
+	}
+
+	if strings.Compare(networkStackOrder[a[i].NetworkStack], networkStackOrder[a[j].NetworkStack]) < 0 {
+		return true
+	} else if strings.Compare(networkStackOrder[a[i].NetworkStack], networkStackOrder[a[j].NetworkStack]) > 0 {
+		return false
+	}
Evidence
In OrderedJobVariants.Less(), the comparator maps known network stacks to ranks "0".."3" and
compares those ranks. For an unknown NetworkStack, networkStackOrder[a[i].NetworkStack] returns
the zero value "", which compares less than "0", causing unknown values to sort ahead of the known
ordering. This affects both HTML and Markdown variant column ordering because both paths use this
comparator via sort.Sort(OrderedJobVariants(...)).

tools/codegen/cmd/featuregate-test-analyzer.go[640-653]
tools/codegen/cmd/featuregate-test-analyzer.go[263-268]
tools/codegen/cmd/featuregate-test-analyzer.go[417-421]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
Unknown `NetworkStack` values currently get rank "" (zero value) and will sort before the intended ordering. This can make typos/unexpected values appear first and obscure data quality issues.

### Issue Context
`OrderedJobVariants.Less()` uses a rank-map for NetworkStack ordering.

### Fix Focus Areas
- tools/codegen/cmd/featuregate-test-analyzer.go[640-653]

### Suggested fix
Use an explicit rank function with a safe default, e.g. return an `int` rank where unknown values return a large rank (e.g., 99), and optionally break ties by comparing the raw `NetworkStack` strings so ordering remains deterministic even among unknowns.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

ⓘ The new review experience is currently in Beta. Learn more

Grey Divider

Qodo Logo

Comment on lines +655 to 667
if strings.Compare(a[i].OS, a[j].OS) < 0 {
return true
} else if strings.Compare(a[i].OS, a[j].OS) > 0 {
return false
}

if strings.Compare(a[i].JobTiers, a[j].JobTiers) < 0 {
return true
} else if strings.Compare(a[i].JobTiers, a[j].JobTiers) > 0 {
return false
}

return false
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Action required

1. Optional not in sort 🐞 Bug ≡ Correctness

OrderedJobVariants.Less() never compares JobVariant.Optional, so two distinct variants that
differ only by Optional are treated as equal and sort.Sort may emit them in a non-deterministic
order when the input comes from UnsortedList(). This can break the PR goal of stable HTML/Markdown
column ordering if optional/required variants share the same other fields.
Agent Prompt
### Issue description
`OrderedJobVariants.Less()` does not include `JobVariant.Optional` in its comparison chain. That means two distinct variants that only differ by `Optional` compare equal, and since inputs come from `UnsortedList()` the final ordering can still vary across runs.

### Issue Context
Both HTML and Markdown outputs sort variants derived from a set/map key list.

### Fix Focus Areas
- tools/codegen/cmd/featuregate-test-analyzer.go[621-668]

### Suggested fix
Add a final comparison for `Optional` (and keep it last so it only affects ties), e.g.:
- if `a[i].Optional != a[j].Optional`, return `!a[i].Optional && a[j].Optional` (non-optional first)

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

@openshift-ci
Copy link
Copy Markdown
Contributor

openshift-ci bot commented Apr 2, 2026

@JoelSpeed: all tests passed!

Full PR test history. Your PR dashboard.

Details

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 kubernetes-sigs/prow repository. I understand the commands that are listed here.

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

Labels

size/M Denotes a PR that changes 30-99 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants