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
Bug 1997131: Add Installed badge and update the alert content for upgrading task #9867
Bug 1997131: Add Installed badge and update the alert content for upgrading task #9867
Conversation
9496b01
to
ed28971
Compare
/bugzilla refresh |
1 similar comment
/bugzilla refresh |
@karthikjeeyar: This pull request references Bugzilla bug 1997131, which is valid. 3 validation(s) were run on this bug
No GitHub users were found matching the public email listed for the QA contact in Bugzilla (gamore@redhat.com), skipping review request. In 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 kubernetes/test-infra repository. |
@karthikjeeyar: This pull request references Bugzilla bug 1997131, which is valid. 3 validation(s) were run on this bug
No GitHub users were found matching the public email listed for the QA contact in Bugzilla (gamore@redhat.com), skipping review request. In 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 kubernetes/test-infra repository. |
@karthikjeeyar: An error was encountered querying GitHub for users with public email (gamore@redhat.com) for bug 1997131 on the Bugzilla server at https://bugzilla.redhat.com. No known errors were detected, please see the full error message for details. Full error message.
non-200 OK status code: 403 Forbidden body: "{\n \"documentation_url\": \"https://docs.github.com/en/free-pro-team@latest/rest/overview/resources-in-the-rest-api#secondary-rate-limits\",\n \"message\": \"You have exceeded a secondary rate limit. Please wait a few minutes before you try again.\"\n}\n"
Please contact an administrator to resolve this issue, then request a bug refresh with In 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 kubernetes/test-infra repository. |
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.
lgtm
frontend/packages/pipelines-plugin/src/components/quicksearch/PipelineQuickSearchTaskAlert.tsx
Outdated
Show resolved
Hide resolved
.../packages/pipelines-plugin/src/components/quicksearch/PipelineQuickSearchVersionDropdown.tsx
Outdated
Show resolved
Hide resolved
.../packages/pipelines-plugin/src/components/quicksearch/PipelineQuickSearchVersionDropdown.tsx
Outdated
Show resolved
Hide resolved
.../packages/pipelines-plugin/src/components/quicksearch/PipelineQuickSearchVersionDropdown.tsx
Outdated
Show resolved
Hide resolved
isOpen={isOpen} | ||
toggle={ | ||
<DropdownToggle data-test={'task-version-toggle'} onToggle={toggleIsOpen}> | ||
{versionItems[selectedVersion]} |
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.
This can be undefined, no? Line 21 says:
const versions = item?.attributes?.versions ?? [];
So when it's converted to an object, it could be missing the selectedVersion
"id" -- this would make the toggle empty, no?
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.
I have a check in the caller, so it only renders this VersionDropdown component if its has some value.Now updated to return early if there are no versions available.
frontend/packages/pipelines-plugin/src/components/quicksearch/PipelineQuickSearchDetails.tsx
Outdated
Show resolved
Hide resolved
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.
Some additional comments on the tests
...es/pipelines-plugin/src/components/quicksearch/__tests__/PipelineQuicksearchDetails.spec.tsx
Outdated
Show resolved
Hide resolved
...es/pipelines-plugin/src/components/quicksearch/__tests__/PipelineQuicksearchDetails.spec.tsx
Outdated
Show resolved
Hide resolved
}); | ||
}); | ||
|
||
test('should show the installed badge for the installed tekton hub task', async () => { |
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.
Odd haha, you switched between it
and test
are they not aliases of the same function? 🤔
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.
Yes, its same, will switch back to it
for consistency.
...es/pipelines-plugin/src/components/quicksearch/__tests__/PipelineQuicksearchDetails.spec.tsx
Show resolved
Hide resolved
frontend/packages/pipelines-plugin/src/components/quicksearch/PipelineQuickSearchDetails.tsx
Outdated
Show resolved
Hide resolved
}; | ||
const { getByRole, queryByTestId } = render( | ||
<PipelineQuickSearchDetails | ||
{...{ ...tektonHubProps, selectedItem: installedTektonHubTask }} |
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.
Same comment here about props
...es/pipelines-plugin/src/components/quicksearch/__tests__/PipelineQuicksearchDetails.spec.tsx
Outdated
Show resolved
Hide resolved
...es/pipelines-plugin/src/components/quicksearch/__tests__/PipelineQuicksearchDetails.spec.tsx
Outdated
Show resolved
Hide resolved
...es/pipelines-plugin/src/components/quicksearch/__tests__/PipelineQuicksearchDetails.spec.tsx
Outdated
Show resolved
Hide resolved
...es/pipelines-plugin/src/components/quicksearch/__tests__/PipelineQuicksearchDetails.spec.tsx
Outdated
Show resolved
Hide resolved
aca4244
to
412c2b6
Compare
412c2b6
to
41a7896
Compare
frontend/packages/pipelines-plugin/src/components/quicksearch/PipelineQuickSearchDetails.tsx
Outdated
Show resolved
Hide resolved
frontend/packages/pipelines-plugin/src/components/quicksearch/PipelineQuickSearchDetails.tsx
Outdated
Show resolved
Hide resolved
.../packages/pipelines-plugin/src/components/quicksearch/PipelineQuickSearchVersionDropdown.tsx
Outdated
Show resolved
Hide resolved
.../packages/pipelines-plugin/src/components/quicksearch/PipelineQuickSearchVersionDropdown.tsx
Outdated
Show resolved
Hide resolved
.../packages/pipelines-plugin/src/components/quicksearch/PipelineQuickSearchVersionDropdown.tsx
Outdated
Show resolved
Hide resolved
.../packages/pipelines-plugin/src/components/quicksearch/PipelineQuickSearchVersionDropdown.tsx
Outdated
Show resolved
Hide resolved
41a7896
to
bd39827
Compare
.../packages/pipelines-plugin/src/components/quicksearch/PipelineQuickSearchVersionDropdown.tsx
Outdated
Show resolved
Hide resolved
bd39827
to
0c5259e
Compare
.../packages/pipelines-plugin/src/components/quicksearch/PipelineQuickSearchVersionDropdown.tsx
Outdated
Show resolved
Hide resolved
0c5259e
to
27c4ee1
Compare
27c4ee1
to
2dfb387
Compare
@karthikjeeyar: This pull request references Bugzilla bug 1997131, which is valid. 3 validation(s) were run on this bug
No GitHub users were found matching the public email listed for the QA contact in Bugzilla (gamore@redhat.com), skipping review request. In 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 kubernetes/test-infra repository. |
2dfb387
to
06f0b05
Compare
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.
Thanks @karthikjeeyar, Verified the changes. And it works as expected.
/lgtm
frontend/packages/pipelines-plugin/src/components/catalog/providers/useTasksProvider.tsx
Outdated
Show resolved
Hide resolved
...tend/packages/pipelines-plugin/src/components/catalog/providers/useTekonHubTasksProvider.tsx
Show resolved
Hide resolved
.../packages/pipelines-plugin/src/components/quicksearch/PipelineQuickSearchVersionDropdown.tsx
Outdated
Show resolved
Hide resolved
06f0b05
to
d4eb7b2
Compare
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.
/lgtm
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: andrewballantyne, bgliwa01, karthikjeeyar, vikram-raj 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 |
@karthikjeeyar: All pull requests linked via external trackers have merged: Bugzilla bug 1997131 has been moved to the MODIFIED state. In 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 kubernetes/test-infra repository. |
Fixes:
https://issues.redhat.com/browse/ODC-6269
Analysis / Root cause:
Update the content of quicksearch modal as per final design.
Solution Description:
Screen shots / Gifs for design review:
Unit test coverage report:
Test setup:
Browser conformance:
cc: @bgliwa01