Repository navigation
fix(dashboard): drop the resource map ConfigMaps once the map is unreadable - #4789
Conversation
…adable React Query keeps the last successful list when a refetch fails. After the resource map became forbidden or went away, the hook went on parsing that old copy, so the ConfigMaps tab stayed with stale names, and a malformed map kept its old error, instead of the tab going away as it does when the map is unreadable from the start. Assisted-by: LLM Signed-off-by: Yan Bondarenko <[email protected]>
github-actions
Bot
added
area/dashboard
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (3)
Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 7 remain after this review. 📝 WalkthroughWalkthroughThe ConfigMap hook ignores resource-map data after 403 or 404 errors. Tests cover the resulting ConfigMaps tab visibility and displayed content. ChangesConfigMap access error handling
Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~10 minutes Change: Bug fix Merge Risk: ⚪ Minimal · up to No actionable issue remains in the supplied evidence; the change is mergeable after normal checks. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Aleksei Sviridkin (lexfrei)
left a comment
Aleksei Sviridkin (lexfrei)
left a comment
There was a problem hiding this comment.
europrinter (@yankawai) LGTM. React Query keeps the last successful data after a failed refetch, so the stale names were real, and moving the 403/404 check in front of the parse is the smallest fix for it.
On a trial merge with current main the detail route tests pass, typecheck is clean and lint stays at the three existing warnings. With the hook reverted to main, all four new tests fail, so they pin the fix.
CI hasn't run on this head yet, every workflow is waiting for approval.
2 participants
What this PR does
Follow-up to #4612.
The ConfigMaps tab takes its names from the application's resource map. React Query keeps the last successful list when a refetch fails, so when the resource map became forbidden or went away after a successful read,
useApplicationConfigMapswent on parsing the old copy. The tab stayed with the old names, every listed ConfigMap showed an access error, and a malformed map kept its old error. A map that is unreadable from the start already hid the tab.Now a 403 or 404 on the resource map drops the names and the error whether or not an earlier read succeeded, so the tab goes away in both cases. Any other error is still shown on the tab, as before.
The new tests are a page test with stale resource map data next to a 403, and hook tests that read the map and then refetch it with 403 and with 404, plus a malformed map followed by a 403. All four fail on main and pass here.
pnpm typecheck,pnpm test(451 tests in 62 files) andpnpm lintpass.Screenshots
No visual change. Once the resource map is unreadable, the page looks the same as for an application whose map was never readable, with no ConfigMaps tab. The tab itself is shown in #4612.
Downstream repositories
Walked the trigger map against the diff. It touches only the console hook that reads the resource map and its tests. No chart, values, schema, CRD or
hack/file changes, and the console is vendored in this repository.Release note
Summary by CodeRabbit