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
|
2 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.