feat: add Image Updater to GitOPS console#244
Conversation
Signed-off-by: dkarpele <karpelevich@gmail.com>
|
@coderabbitai do you have any thoughts? inital review of the changes? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Get PR details and file list
gh pr view 244 --repo redhat-developer/gitops-console-plugin --json title,body,files,additions,deletions,changedFiles 2>/dev/null | jq '{title, additions, deletions, changedFiles, files: [.files[].path]}'Length of output: 1356 🏁 Script executed: #!/bin/bash
# Get the diff for the PR
gh pr diff 244 --repo redhat-developer/gitops-console-plugin 2>/dev/null | head -2000Length of output: 67810 PR description images show:
🔴 Bugs1. Multi-select filter short-circuits incorrectly ( When a user selects both "Has Apps" and "No Apps" (or both "Ready" and "Not Ready"), the filter should return // Current — if both 'has-apps' and 'no-apps' are in input.selected,
// only the first if-branch fires (returns hasApps), never checking the second
filter: (input, item) => {
if (input.selected?.length) {
const hasApps = apps > 0;
if (input.selected.includes('has-apps')) { return hasApps; } // ← short-circuits
if (input.selected.includes('no-apps')) { return !hasApps; }
}
return true;
},Suggested fix: filter: (input, item) => {
if (!input.selected?.length) return true;
const apps = (item as ImageUpdaterKind).status?.applicationsMatched;
const hasApps = apps > 0;
return (
(input.selected.includes('has-apps') && hasApps) ||
(input.selected.includes('no-apps') && !hasApps)
);
},Same pattern applies to the 2. Direct prop mutation ( // ❌ mutates the destructured parameter
if (listAllNamespaces) {
namespace = null;
}Use a local variable instead: const effectiveNamespace = listAllNamespaces ? null : namespace;
// then use effectiveNamespace everywhere below🟠 Medium Issues3. Missing The // ❌ missing flags
{
"type": "console.navigation/resource-ns",
"properties": {
"id": "imageupdater",
...
}
}// ✅ add flags guard
{
"type": "console.navigation/resource-ns",
"properties": { "id": "imageupdater", ... },
"flags": { "required": ["IMAGEUPDATER"] }
}The separator below it should also carry the same guard. 4. Hardcoded GVK literals instead of // ❌ hardcoded in two places
groupVersionKind: {
group: 'argocd-image-updater.argoproj.io',
kind: 'ImageUpdater',
version: 'v1alpha1',
},// ✅ use the model
groupVersionKind: modelToGroupVersionKind(ImageUpdaterModel),5. Import style inconsistency ( The file mixes // ❌ inconsistent with the rest of the file
import ImageUpdaterDetailsTab from "`@gitops/components/imageupdater/ImageUpdaterDetailsTab`";
import ImageUpdaterRecentUpdatesTab from "`@gitops/components/imageupdater/ImageUpdaterRecentUpdatesTab`";Should be: import ImageUpdaterDetailsTab from './ImageUpdaterDetailsTab';
import ImageUpdaterRecentUpdatesTab from './ImageUpdaterRecentUpdatesTab';6.
7. Loose equality checks // ❌ use === instead of ==
if (imageUpdaterList == undefined || imageUpdaterList.length == 0)
if (showTitle == undefined)🟡 Minor / Style8. IIFE in JSX ( The // ❌
{(() => {
const readyCondition = status.conditions?.find((c) => c.type === 'Ready');
if (!readyCondition) return '-';
return readyCondition.status === 'True' ? t('True') : t('False');
})()}
// ✅
{status.conditions?.find((c) => c.type === 'Ready')
? status.conditions.find((c) => c.type === 'Ready')!.status === 'True' ? t('True') : t('False')
: '-'}
// or extract to a variable above the return9. The guard is currently placed near the bottom of the component body, after all hooks ( 10. PF class names
11.
✅ What's done well
|
| if (listAllNamespaces) { | ||
| namespace = null; | ||
| } |
There was a problem hiding this comment.
We should avoid mutating function parameters here may be something like?
| if (listAllNamespaces) { | |
| namespace = null; | |
| } | |
| const effectiveNamespace = listAllNamespaces ? null : namespace; |
| import ResourceYAMLTab from '../shared/ResourceYAMLTab/ResourceYAMLTab'; | ||
|
|
||
| import { useImageUpdaterActionsProvider } from './hooks/useImageUpdaterActionsProvider'; | ||
| import ImageUpdaterDetailsTab from "@gitops/components/imageupdater/ImageUpdaterDetailsTab"; |
There was a problem hiding this comment.
use single quotes here for consistency, same as line 14
| namespace: string | undefined, | ||
| ): DataViewTr[] => { | ||
| const rows: DataViewTr[] = []; | ||
| if (imageUpdaterList == undefined || imageUpdaterList.length == 0) { |
There was a problem hiding this comment.
| if (imageUpdaterList == undefined || imageUpdaterList.length == 0) { | |
| if (imageUpdaterList === undefined || imageUpdaterList.length === 0) { |
| title={t('Applications Matched')} | ||
| help={t('Number of applications matched by this ImageUpdater.')} | ||
| > | ||
| {status.applicationsMatched != null ? String(status.applicationsMatched) : '-'} |
There was a problem hiding this comment.
Consider using ?? operator here and the lines below instead?
| const useColumnsDV = (getSortParams: (columnIndex: number) => ThProps['sort']): DataViewTh[] => { | ||
| const { t } = useTranslation('plugin__gitops-plugin'); |
There was a problem hiding this comment.
For consistency, prefer useGitOpsTranslation() throughout the file.
| const useColumnsDV = (getSortParams: (columnIndex: number) => ThProps['sort']): DataViewTh[] => { | |
| const { t } = useTranslation('plugin__gitops-plugin'); | |
| +const useColumnsDV = ( | |
| + getSortParams: (columnIndex: number) => ThProps['sort'], | |
| + t: (key: string) => string, | |
| +): DataViewTh[] => { |
then update the line 34!
|
|
||
| const { sortBy, direction, getSortParams } = useGitOpsDataViewSort(columnSortConfig); | ||
|
|
||
| const columnsDV = useColumnsDV(getSortParams); |
There was a problem hiding this comment.
| const columnsDV = useColumnsDV(getSortParams); | |
| const columnsDV = useColumnsDV(getSortParams, t); |
| if (!sortBy || !direction) return data; | ||
|
|
||
| return [...data].sort((a, b) => { | ||
| let aValue: any, bValue: any; |
There was a problem hiding this comment.
Lines 159 and throughout the switch statement use any for aValue and bValue. Consider more specific types for sort values for better type safety.
| { | ||
| id: 'last-checked', | ||
| cell: obj.status?.lastCheckedAt ? ( | ||
| <div style={{ whiteSpace: 'nowrap' }}> |
There was a problem hiding this comment.
Inline styles at lines 381, 397, and 409. Consider moving them to the SCSS file.
Also might apply to lines: 395-398, 407-416
|
Ahh I see Coderabbit has some of my review comments, I got them after refreshing the page. Looks good just a few comments! my cluster died when I was testing. I will run another quick test when I am back. |
Signed-off-by: dkarpele <karpelevich@gmail.com>
|
Thanks for the review Atif! |
|
Warning Review limit reached
More reviews will be available in 25 minutes and 34 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThis PR adds complete console plugin support for the Argo CD Image Updater resource type, including TypeScript data models, React UI components for listing and viewing ImageUpdaters, console extension wiring with navigation and pages, and internationalized UI strings across multiple locales. ChangesImageUpdater Console Plugin Support
🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (1)
src/gitops/components/imageupdater/ImageUpdaterRecentUpdatesTab.tsx (1)
190-194: ⚡ Quick winSimplify nested ternary operators for readability.
The nested ternary expressions reduce code clarity and are flagged by ESLint's
no-nested-ternaryrule. Using if-else statements makes the comparison logic more explicit and easier to maintain.♻️ Proposed refactor to use if-else
- if (direction === 'asc') { - return aValue < bValue ? -1 : aValue > bValue ? 1 : 0; - } else { - return aValue > bValue ? -1 : aValue < bValue ? 1 : 0; - } + if (aValue < bValue) { + return direction === 'asc' ? -1 : 1; + } + if (aValue > bValue) { + return direction === 'asc' ? 1 : -1; + } + return 0;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/gitops/components/imageupdater/ImageUpdaterRecentUpdatesTab.tsx` around lines 190 - 194, The comparison block in ImageUpdaterRecentUpdatesTab (the code that returns sort order based on direction and aValue/bValue) uses nested ternary operators which hurts readability; replace the nested ternaries with explicit if/else branches so the logic is clearer: check if direction === 'asc' then use if (aValue < bValue) return -1; else if (aValue > bValue) return 1; else return 0; and for the else (descending) branch use if (aValue > bValue) return -1; else if (aValue < bValue) return 1; else return 0; so the comparison logic around aValue, bValue and direction is identical but written with clear if/else statements.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@console-extensions.json`:
- Around line 391-397: There are duplicate console.navigation/separator entries
using the same properties.id "argocd-separator" which can cause unstable nav
behavior; locate the console.navigation/separator blocks (look for type
"console.navigation/separator" with properties.perspective "admin" and section
"gitops-navigation-section") and give each properties.id a unique value (e.g.,
"argocd-separator" and "argocd-separator-2" or a semantic variant) so they no
longer collide, ensuring any insertAfter references (like "imageupdater") remain
correct.
In `@locales/ja/plugin__gitops-plugin.json`:
- Around line 108-154: The new ImageUpdater UI strings (e.g., "Edit
ImageUpdater", "Delete ImageUpdater", "ImageUpdater details", "Images Managed",
"Last Checked At", "There are no ImageUpdaters in this namespace.", etc.) are
still in English for the ja/ko/zh locales — update the locale JSON entries for
these keys in the Japanese (ja), Korean (ko), and Chinese (zh)
plugin__gitops-plugin.json files with proper translations (or mark them for
follow-up), ensuring each English key is replaced by the corresponding
translated string and preserves the exact JSON keys and punctuation.
In `@plugin-metadata.ts`:
- Around line 22-23: The newly added module paths for ImageUpdaterList and
ImageUpdaterDetailsPage use double quotes and violate the project's prettier
quote style; update the string literals in plugin-metadata.ts for the keys
ImageUpdaterList and ImageUpdaterDetailsPage to use single quotes (i.e., change
"./gitops/..." to './gitops/...') so the file conforms to the configured
lint/prettier rules.
In `@src/gitops/components/imageupdater/imageupdater-list.scss`:
- Around line 2-22: The truncation rules currently target all PatternFly tables
via selectors like ".pf-c-table tbody td[data-label='Name'], .pf-c-table tbody
td[data-label='Namespace']" and ".pf-c-table tbody
td[data-label='Description']"; scope these rules to the ImageUpdater list by
prefixing both selector groups with the ImageUpdater wrapper class (for example
".image-updater-list" or the component's top-level class used in the
ImageUpdater list). Update the two selector blocks so they read "<wrapper>
.pf-c-table tbody td[data-label='Name'], <wrapper> .pf-c-table tbody
td[data-label='Namespace']" and "<wrapper> .pf-c-table tbody
td[data-label='Description']", preserving the existing properties (white-space,
overflow, text-overflow, and inner a/.co-resource-item styles) and keep the
!important only if necessary for specificity.
In `@src/gitops/components/imageupdater/ImageUpdaterList.tsx`:
- Around line 58-68: The sort-key list in columnSortConfig inserts 'namespace'
incorrectly because the ternary condition is inverted; change the logic so
'namespace' is only included when listAllNamespaces is true (i.e., include
['namespace'] when listAllNamespaces is true, otherwise []). Update the
expression building the array in columnSortConfig (the place that currently uses
!listAllNamespaces || !effectiveNamespace || effectiveNamespace === '' to decide
insertion) to use listAllNamespaces ? ['namespace'] : [], and ensure the
React.useMemo dependencies still include listAllNamespaces (and
effectiveNamespace only if you keep it in the condition).
In `@src/gitops/components/imageupdater/ImageUpdaterRecentUpdatesTab.tsx`:
- Around line 18-42: The null-check for obj is after several React hook calls
which violates the Rules of Hooks; move the early return (if (!obj) return null)
to the very top of the ImageUpdaterRecentUpdatesTab component so that no hooks
(useGitOpsTranslation, useGitOpsDataViewSort, useColumnsDV, useRowsDV,
React.useMemo/sortData) are invoked when obj is null, preserving unconditional
hook order and preventing "rendered fewer hooks" errors.
---
Nitpick comments:
In `@src/gitops/components/imageupdater/ImageUpdaterRecentUpdatesTab.tsx`:
- Around line 190-194: The comparison block in ImageUpdaterRecentUpdatesTab (the
code that returns sort order based on direction and aValue/bValue) uses nested
ternary operators which hurts readability; replace the nested ternaries with
explicit if/else branches so the logic is clearer: check if direction === 'asc'
then use if (aValue < bValue) return -1; else if (aValue > bValue) return 1;
else return 0; and for the else (descending) branch use if (aValue > bValue)
return -1; else if (aValue < bValue) return 1; else return 0; so the comparison
logic around aValue, bValue and direction is identical but written with clear
if/else statements.
🪄 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: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: 2fe87c26-f7ba-4849-b90c-aa12ca8e5278
📒 Files selected for processing (17)
console-extensions.jsonlocales/en/plugin__gitops-plugin.jsonlocales/ja/plugin__gitops-plugin.jsonlocales/ko/plugin__gitops-plugin.jsonlocales/zh/plugin__gitops-plugin.jsonplugin-metadata.tssrc/gitops/components/imageupdater/ImageUpdaterDetailsPage.tsxsrc/gitops/components/imageupdater/ImageUpdaterDetailsTab.tsxsrc/gitops/components/imageupdater/ImageUpdaterList.tsxsrc/gitops/components/imageupdater/ImageUpdaterListTab.tsxsrc/gitops/components/imageupdater/ImageUpdaterNavPage.tsxsrc/gitops/components/imageupdater/ImageUpdaterRecentUpdatesTab.tsxsrc/gitops/components/imageupdater/hooks/useImageUpdaterActionsProvider.tsxsrc/gitops/components/imageupdater/imageupdater-list.scsssrc/gitops/models/ImageUpdaterModel.tssrc/gitops/templates/imageupdater-yaml.tssrc/gitops/templates/index.ts
Signed-off-by: dkarpele <karpelevich@gmail.com>
3 participants
I tested this against Image Updater v1.2.1 that is a part of GitOPS 1.21.
statusfield below is available starting from v1.2.0.