New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add Helm Release List Page #3960
Add Helm Release List Page #3960
Conversation
/assign @christianvogt |
Adding hold as it depends on #3826 |
/test analyze |
1 similar comment
/test analyze |
/kind feature |
frontend/packages/dev-console/src/components/helm/HelmReleaseRow.tsx
Outdated
Show resolved
Hide resolved
frontend/packages/dev-console/src/components/helm/HelmReleaseHeader.tsx
Outdated
Show resolved
Hide resolved
frontend/packages/dev-console/src/components/helm/helm-utils.ts
Outdated
Show resolved
Hide resolved
e3fc30d
to
35f2d74
Compare
/test analyze |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rohitkrai03 nice job on this!
It feels like the page title/header has more white space than other pages, would you check that out, I might be wrong!
@sspeiche @siamaksade @sbose78 let's verify these are the appropriate columns. This is what we noted during our first meeting and is in the design, but we haven't talked much about it!
Can you also verify that these are the columns displayed during Search of a Helm Release?
Thanks!
getQueryArgument('rowFilter-helm-release-status').split(','); | ||
|
||
const fetchHelmReleases = async () => { | ||
const res: HelmRelease[] = await coFetchJSON('/api/console/helm/list'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rohitkrai03 we've updated /api/console/helm/list
API path to /api/helm/releases
, please update here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ref :
Line 303 in 60ab162
handle("/api/helm/releases", authHandlerWithUser(s.handleHelmList)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@akashshinde Sure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you also update your PR description #3826 with new API endpoints?
Maybe it seems like that because there is no |
35f2d74
to
374aeac
Compare
bf08afa
to
0836298
Compare
Please add unit tests. |
/retest |
@rohitkrai03 you have an error that needs to be addressed:
|
/hold Looks like the plugin type changed on you after you started. |
componentProps: { | ||
name: 'Helm Releases', | ||
href: '/helm-releases', | ||
required: FLAGS.OPENSHIFT, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup, looks like you need to remove this line.
required: FLAGS.OPENSHIFT, |
3fa0224
to
76aaefc
Compare
@andrewballantyne Updated the PR. the plugin SDK changes that went in after #3383 got merged was the issue. Missed that in huge CI logs. |
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: andrewballantyne, christianvogt, divyanshiGupta, rohitkrai03 The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Aritra went to forum-ui and Sam responded... I saw what he looked for and it's just how I look for errors on "Docker Strategy Failed" now 😄 |
/test analyze |
/retest Please review the full test history for this PR and help us cut down flakes. |
@spadgett The change was required for Helm List page. Helm Release is not a Kube resource so we needed to have custom filtering logic for the list page, and in order to re-use the |
The one place I know of that used the checkboxes outside of a k8s resource list is the API explorer. It uses the Agree we should definitely avoid a name collision if there is one. |
Went through the code in API explorer and it seems like some logic is replicated from |
I'm fine with that. Thanks for tracking this down. |
Great, so I'll raise a PR against this BZ https://bugzilla.redhat.com/show_bug.cgi?id=1798096 |
Related Story - https://issues.redhat.com/browse/ODC-2524
Depends on #3826 for the
/helm/list
API.This PR -
Helm Releases
.HelmReleasePage
,HelmReleaseList
,HelmReleaseHeader
andHelmReleaseRow
.HelmReleaseList
calls/helm/list
API based on the namespace and Helm Release secrets fetch by firehose.firehose
resources chaning too often, adds a new utility for deep comparisons on dependencies inuseEffect
hook.TextFilter
component from console.CheckBoxes
component from console. The status based filtering also adds capability to filter multiplw statuses based on combined filterOther
.@openshift/team-devconsole-ux
Screencasts -
Helm Release List and Filtering -
Auto Update List -