NO-JIRA: feat: update the html for feature promotion#2714
NO-JIRA: feat: update the html for feature promotion#2714eggfoobar wants to merge 1 commit intoopenshift:masterfrom
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
the
jira/valid-reference
|
@eggfoobar: This pull request explicitly references no jira issue. 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. |
|
Hello @eggfoobar! Some important instructions when contributing to openshift/api: |
📝 WalkthroughWalkthroughThis change refactors the HTML generation logic for feature gate testing analysis. It replaces direct HTML assembly with a template-based rendering approach. A new utils file introduces data structures (HTMLTemplateData, HTMLFeatureGate, HTMLVariantColumn, HTMLTestRow, HTMLTestCell) and an HTML template constant. The featuregate-test-analyzer command is updated to build structured data via buildHTMLFeatureGateData, render through writeHTMLFromTemplate using the shared template, and output results to file. The hardcoded HTML header constant and blackfriday dependency are removed, with html/template imported to support template-based rendering. 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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.5.0)Error: build linters: unable to load custom analyzer "kubeapilinter": tools/_output/bin/kube-api-linter.so, plugin: not implemented Comment |
openshift-ci
bot
added
the
size/L
ⓘ You are approaching your monthly quota for Qodo. Upgrade your plan Review Summary by QodoEnhance feature promotion HTML with templating and interactive sorting
WalkthroughsDescription• Replace markdown-based HTML generation with Go template system • Add interactive sorting by test name and pass rate percentage • Implement color-coded pass/fail cell visualization for test results • Create structured data types for HTML template rendering Diagramflowchart LR
A["Markdown Generation<br/>blackfriday"] -->|Replace| B["Go Template System<br/>html/template"]
C["Static HTML Header<br/>Constant"] -->|Replace| D["Dynamic Template<br/>with Data Structures"]
E["Test Results Data"] -->|Transform| F["HTMLFeatureGate<br/>Structures"]
F -->|Render| G["Interactive HTML<br/>with Sorting & Colors"]
File Changes1. tools/codegen/cmd/featuregate-test-analyzer.go
|
ⓘ You are approaching your monthly quota for Qodo. Upgrade your plan Code Review by Qodo
1. HTML shows 95%%
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
small quality of life improvement to the html for feature promotion, this addition will allow sorting by pass rate and make it easier to identify where a feature is lacking validation by color coating the pass rate. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: ehila <ehila@redhat.com>
eggfoobar
force-pushed
the
upkeep-updated-feature-verify-html
branch
from
78eecda to
9650576
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tools/codegen/cmd/featuregate-test-analyzer.go (1)
222-235: Make column ordering deterministic across NetworkStack variants.Line 224 uses
OrderedJobVariants, which doesn’t compareNetworkStack. When Topology/Cloud/Architecture match (ipv4/ipv6/dual), the sort order can fluctuate, changingColIndexbetween runs and making the report harder to scan. Consider addingNetworkStackas a final tie-breaker (or use a stable sort).🔧 Suggested comparator update (outside this block)
func (a OrderedJobVariants) Less(i, j int) bool { if strings.Compare(a[i].Topology, a[j].Topology) < 0 { return true } else if strings.Compare(a[i].Topology, a[j].Topology) > 0 { return false } if strings.Compare(a[i].Cloud, a[j].Cloud) < 0 { return true } else if strings.Compare(a[i].Cloud, a[j].Cloud) > 0 { return false } if strings.Compare(a[i].Architecture, a[j].Architecture) < 0 { return true } else if strings.Compare(a[i].Architecture, a[j].Architecture) > 0 { return false } + + if strings.Compare(a[i].NetworkStack, a[j].NetworkStack) < 0 { + return true + } else if strings.Compare(a[i].NetworkStack, a[j].NetworkStack) > 0 { + return false + } return false }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/codegen/cmd/featuregate-test-analyzer.go` around lines 222 - 235, The current sorting of jobVariants uses OrderedJobVariants which doesn't include NetworkStack, causing nondeterministic ColIndex when Topology/Cloud/Architecture tie; update the comparator used by OrderedJobVariants (the comparator implementation for OrderedJobVariants) to include NetworkStack as the final tie-breaker (or switch to a stable sort while adding NetworkStack comparison) so jobVariants (the unsorted list produced from jobVariantsSet) is deterministically ordered before building variants (utils.HTMLVariantColumn) and assigning ColIndex.tools/codegen/pkg/utils/html.go (1)
89-162: Add keyboard support for sortable headers.Line 89 and Line 91 create clickable
<th>elements, but they’re mouse-only. Consider addingtabindex/roleand a key handler (Enter/Space) so sorting is accessible via keyboard.♿ Proposed accessibility tweak
- <th class="sortable sort-indicator" data-table="{{$fgIdx}}" data-col="0" data-sort="text">Test Name</th> + <th class="sortable sort-indicator" data-table="{{$fgIdx}}" data-col="0" data-sort="text" role="button" tabindex="0">Test Name</th> ... - <th class="sortable sort-indicator" data-table="{{$fgIdx}}" data-col="{{$v.ColIndex}}" data-sort="percent"> + <th class="sortable sort-indicator" data-table="{{$fgIdx}}" data-col="{{$v.ColIndex}}" data-sort="percent" role="button" tabindex="0"> ... - document.querySelectorAll('th.sortable').forEach(function(th) { - th.addEventListener('click', function() { - sortTable(this.dataset.table, parseInt(this.dataset.col), this.dataset.sort, this); - }); - }); + document.querySelectorAll('th.sortable').forEach(function(th) { + function invokeSort() { + sortTable(th.dataset.table, parseInt(th.dataset.col), th.dataset.sort, th); + } + th.addEventListener('click', invokeSort); + th.addEventListener('keydown', function(e) { + if (e.key === 'Enter' || e.key === ' ') { + e.preventDefault(); + invokeSort(); + } + }); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/codegen/pkg/utils/html.go` around lines 89 - 162, The sortable table headers are only clickable by mouse; update the <th class="sortable"> generation to include tabindex="0" and role="button" (where headers are created in the template loop for $fg.Variants and the Test Name header) and add a keydown handler alongside the existing click handler in the DOMContentLoaded setup to call sortTable when Enter (keyCode 13 or key === 'Enter') or Space (keyCode 32 or key === ' ') is pressed (call sortTable with this.dataset.table, parseInt(this.dataset.col), this.dataset.sort, this), ensuring you preventDefault for Space to avoid scrolling; keep the existing sortTable function unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tools/codegen/cmd/featuregate-test-analyzer.go`:
- Around line 222-235: The current sorting of jobVariants uses
OrderedJobVariants which doesn't include NetworkStack, causing nondeterministic
ColIndex when Topology/Cloud/Architecture tie; update the comparator used by
OrderedJobVariants (the comparator implementation for OrderedJobVariants) to
include NetworkStack as the final tie-breaker (or switch to a stable sort while
adding NetworkStack comparison) so jobVariants (the unsorted list produced from
jobVariantsSet) is deterministically ordered before building variants
(utils.HTMLVariantColumn) and assigning ColIndex.
In `@tools/codegen/pkg/utils/html.go`:
- Around line 89-162: The sortable table headers are only clickable by mouse;
update the <th class="sortable"> generation to include tabindex="0" and
role="button" (where headers are created in the template loop for $fg.Variants
and the Test Name header) and add a keydown handler alongside the existing click
handler in the DOMContentLoaded setup to call sortTable when Enter (keyCode 13
or key === 'Enter') or Space (keyCode 32 or key === ' ') is pressed (call
sortTable with this.dataset.table, parseInt(this.dataset.col),
this.dataset.sort, this), ensuring you preventDefault for Space to avoid
scrolling; keep the existing sortTable function unchanged.
|
@eggfoobar: 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. |
| summaryMarkdown := md.ExactBytes() | ||
| if len(o.OutputDir) > 0 { | ||
| filename := filepath.Join(o.OutputDir, "feature-promotion-summary.md") | ||
| if err := os.WriteFile(filename, summaryMarkdown, 0644); err != nil { | ||
| errs = append(errs, err) | ||
| } |
There was a problem hiding this comment.
Not blocking for this PR, but I wonder if we oughta remove the additional markdown file artifact? I never knew it existed until now, and I suspect it isn't actually used all that often.
It doesn't hurt to keep it, but if it isn't actually buying us anything it also seems like we could reduce a failure mode if it can't write the markdown file.
@JoelSpeed Do you know of any specific uses of the markdown file artifact for feature promotion runs?
There was a problem hiding this comment.
Nothing specific that I know of no
everettraven
left a comment
everettraven
left a comment
There was a problem hiding this comment.
This seems like a worthwhile change and I don't have any specific concerns about the implementation here.
/lgtm
|
Pipeline controller notification No second-stage tests were triggered for this PR. This can happen when:
Use |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: everettraven 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 |
openshift-ci
bot
added
the
approved
4 participants
small quality of life improvement to the html for feature promotion, this addition will allow sorting by pass rate and make it easier to identify where a feature is lacking validation by color coating the pass rate.
Example: