-
Notifications
You must be signed in to change notification settings - Fork 605
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
Tekton hub integration #9771
Tekton hub integration #9771
Conversation
e938cfe
to
f83430b
Compare
frontend/packages/console-shared/src/components/quick-search/QuickSearchList.tsx
Outdated
Show resolved
Hide resolved
frontend/packages/console-shared/src/components/quick-search/QuickSearchList.tsx
Outdated
Show resolved
Hide resolved
@@ -0,0 +1 @@ | |||
export const TEKTON_HUB_API_ENDPOINT = 'https://api.hub.tekton.dev'; |
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.
isn't this endpoint id there within operator, so that we can read and could be dynamic
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.
Backend support for reading the endpoints is not there yet, so for now we are going with tektonhub API
frontend/packages/pipelines-plugin/src/components/catalog/hooks/useTaskCategories.ts
Outdated
Show resolved
Hide resolved
frontend/packages/pipelines-plugin/src/components/catalog/hooks/useTaskResources.ts
Outdated
Show resolved
Hide resolved
...tend/packages/pipelines-plugin/src/components/catalog/providers/useTekonHubTasksProvider.tsx
Outdated
Show resolved
Hide resolved
.../packages/pipelines-plugin/src/components/pipelines/pipeline-builder/PipelineBuilderForm.tsx
Outdated
Show resolved
Hide resolved
frontend/packages/pipelines-plugin/src/components/quicksearch/pipeline-quicksearch-utils.ts
Outdated
Show resolved
Hide resolved
} | ||
} else { | ||
resolve(savedCallback.current({ metadata: { name: item.data.name } })); | ||
createTask(selectedVersionUrl, namespace).catch(() => |
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.
no action needed on then
here only on catch
?
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, In case of successful task creation, the resources will be created and pulled in automatically by K8swatchers.
frontend/packages/pipelines-plugin/src/components/quicksearch/PipelineQuickSearchDetails.scss
Outdated
Show resolved
Hide resolved
1ac673f
to
040c056
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.
Also we talked about the storage of the loading tasks, please make those changes as discussed.
frontend/packages/console-shared/src/components/quick-search/QuickSearchDetails.scss
Outdated
Show resolved
Hide resolved
} | ||
|
||
const QuickSearchDetails: React.FC<QuickSearchDetailsProps> = ({ selectedItem, closeModal }) => { | ||
const DefaultContent = ({ selectedItem, closeModal }) => { |
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.
We should type this with the props, it's a stand-alone function.
(props: QuickSearchDetailsRendererProps) => React.ReactNode
maybe can be it's own type and we just reuse it here?
Also, maybe we make this lower case. It's not a component, it's a defaultContentRenderer
. Perhaps the rename and go with camelCase as it's a function.
frontend/packages/console-shared/src/components/quick-search/QuickSearchList.tsx
Outdated
Show resolved
Hide resolved
frontend/packages/console-shared/src/components/quick-search/QuickSearchList.tsx
Outdated
Show resolved
Hide resolved
frontend/packages/console-shared/src/components/quick-search/QuickSearchList.tsx
Outdated
Show resolved
Hide resolved
...tend/packages/pipelines-plugin/src/components/catalog/providers/useTekonHubTasksProvider.tsx
Outdated
Show resolved
Hide resolved
...gin/src/components/pipelines/detail-page-tabs/pipeline-details/PipelineVisualizationTask.tsx
Outdated
Show resolved
Hide resolved
frontend/packages/pipelines-plugin/src/components/quicksearch/PiplineQuickSearchDetails.tsx
Outdated
Show resolved
Hide resolved
React.useEffect(() => { | ||
savedCallback.current = callback; | ||
}, [callback]); |
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.
Just move this next to the useRef... you're just holding onto it and not doing any computation, no reason to use an effect.
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.
Couple other things...
Somehow the Sidebar styles got mucked up. I think it was in your previous PR (or another PR that I'm unaware of that touched the Pipeline Builder):
Also not sure how it happened... I was clicking around in the modal and I got this:
I also got a ton of "Failed to fetch" errors from the useTekonHubTasksProvider.tsx
file.
Your implementation also is not what we discussed. I'm not sure if that's because it didn't work or because you misunderstood. I don't want to hold up your PR beyond this sprint, if you can fix the UID issue above and other issues I mentioned, we can merge this as-is.
Seek another review for the LGTM when you get a chance, hopefully we can merge this before EOD tomorrow.
const tags = annotations[TektonTaskAnnotation.tags]?.split(/\s*,\s*/) || []; | ||
const categories = annotations[TektonTaskAnnotation.categories]?.split(/\s*,\s*/) || []; |
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.
Sorry I didn't see this before... but I wonder if we don't want to use parseJSONAnnotation... not sure if that's the goal of what you're trying to do or not.
I'm unsure what format these annotations are in...
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.
annotations are strings, could be comma separated, so splitting the string into array.
@@ -42,6 +42,7 @@ export type PipelineResult = { | |||
}; | |||
|
|||
export type PipelineTask = { | |||
metadata?: { installing?: boolean }; |
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 is no longer needed, right?
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.
Oops, missed this spot.
d0c5e77
to
8b6994a
Compare
Fixed TaskSidebar alignment issue caused by this removing bootstrap close styles commit 70a267f Not able to reproduce the uid of undefined error, It seems like some flake happened in catalogServiceProvider while loading extensions.
There were a lot of failures for |
30045fb
to
126ace2
Compare
|
||
React.useEffect(() => { | ||
const { loadingTasks } = values.formData; | ||
loadingTasks.map((task) => { |
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.
nit: will forEach be a better fit here
frontend/packages/pipelines-plugin/src/components/pipelines/pipeline-builder/utils.ts
Outdated
Show resolved
Hide resolved
export const isTaskVersionInstalled = (item: CatalogItem): boolean => { | ||
if (item.attributes?.installed) { | ||
return item.attributes.installed !== ''; | ||
} | ||
return false; | ||
}; |
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.
nit: can we have it like this?
export const isTaskVersionInstalled = (item: CatalogItem): boolean => { | |
if (item.attributes?.installed) { | |
return item.attributes.installed !== ''; | |
} | |
return false; | |
}; | |
export const isTaskVersionInstalled = (item: CatalogItem): boolean => !!item.attributes?.installed; |
export const isInstalledNamespaceTask = (item: CatalogItem) => { | ||
return ( | ||
item.data.kind === TaskModel.kind && | ||
item.data?.metadata?.annotations[TektonTaskAnnotation.installedFrom] === TEKTONHUB |
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.
item.data?.metadata?.annotations[TektonTaskAnnotation.installedFrom] === TEKTONHUB | |
item.data.metadata?.annotations[TektonTaskAnnotation.installedFrom] === TEKTONHUB |
if (!item?.attributes?.versions) { | ||
return null; | ||
} | ||
return item?.attributes?.versions.find((v) => v.id.toString() === version)?.rawURL; |
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.
return item?.attributes?.versions.find((v) => v.id.toString() === version)?.rawURL; | |
return item.attributes.versions.find((v) => v.id.toString() === version)?.rawURL; |
i.name === item.name && | ||
item.data.kind !== ClusterTaskModel.kind && | ||
i.data.kind === TaskModel.kind && | ||
i.data?.metadata?.annotations[TektonTaskAnnotation.installedFrom] === TEKTONHUB, |
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 as above
126ace2
to
4296e7b
Compare
/approve Verified the changes, works as expected cc @andrewballantyne |
/retest |
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
Adding on behalf of the docs & px team as the Epic already contains the acks. |
4296e7b
to
12878fa
Compare
Rebased the PR to resolve conflicts. |
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: andrewballantyne, invincibleJai, karthikjeeyar 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 |
Fixes:
https://issues.redhat.com/browse/ODC-6134
https://issues.redhat.com/browse/ODC-6135
Analysis / Root cause:
Integrate tektonHub tasks in the pipeline builder, where users can find/install and update a tekton hub tasks in a namespace.
Solution Description:
Todo:
Screen shots / Gifs for design review:
Add already Installed Task flow:
Install task flow:
Update task flow:
Installing and Delete
Add task
Decorators:Unit test coverage report:
Test setup:
Browser conformance:
cc: @serenamarie125 @bgliwa01 @andrewballantyne