Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Migrate pipelines plugin static extension to dynamic #9842

Merged
merged 1 commit into from Aug 27, 2021
Merged

Migrate pipelines plugin static extension to dynamic #9842

merged 1 commit into from Aug 27, 2021

Conversation

debsmita1
Copy link
Contributor

Jira story:
https://issues.redhat.com/browse/ODC-6197

Solution description:
migrate static extensions that are being used in plugin.ts to use dynamic extensions in console-extensions.json

@openshift-ci openshift-ci bot added the component/pipelines Related to pipelines-plugin label Aug 19, 2021
@debsmita1
Copy link
Contributor Author

/assign @rohitkrai03

@openshift-ci openshift-ci bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 20, 2021
@openshift-ci openshift-ci bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 20, 2021
@debsmita1
Copy link
Contributor Author

/cc @karthikjeeyar

karthikjeeyar
karthikjeeyar reviewed Aug 25, 2021
View reviewed changes
Copy link
Contributor

@karthikjeeyar karthikjeeyar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could move few more entries to dynamic extension.

frontend/packages/pipelines-plugin/package.json Outdated
Comment on lines 27 to 36
"pipelinesComponent": "src/components/pipelines",
"pipelineRunsComponent": "src/components/pipelineruns",
"taskRunsComponent": "src/components/taskruns",
"repositoryComponent": "src/components/repository",
"clusterTasksComponent": "src/components/cluster-tasks",
"tasksComponent": "src/components/tasks",
"conditionsComponent": "src/components/conditions",
"pipelineComponent": "src/components",
"pipelinesListsComponent": "src/components/pipelines-lists",
"triggersListsComponent": "src/components/triggers-lists"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead have a index.ts in src/components that exports all these components and expose it as pipelineComponents like below.

Suggested change
"pipelinesComponent": "src/components/pipelines",
"pipelineRunsComponent": "src/components/pipelineruns",
"taskRunsComponent": "src/components/taskruns",
"repositoryComponent": "src/components/repository",
"clusterTasksComponent": "src/components/cluster-tasks",
"tasksComponent": "src/components/tasks",
"conditionsComponent": "src/components/conditions",
"pipelineComponent": "src/components",
"pipelinesListsComponent": "src/components/pipelines-lists",
"triggersListsComponent": "src/components/triggers-lists"
"pipelineComponents": "src/components",

frontend/packages/pipelines-plugin/console-extensions.json Outdated
"/k8s/ns/:ns/tekton.dev~v1alpha1~PipelineRun"
],
"component": {
"$codeRef": "pipelineRunsComponent.PipelineRunsPage"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

based on my previous comment. (update in all other occurrences in this file)

Suggested change
"$codeRef": "pipelineRunsComponent.PipelineRunsPage"
"$codeRef": "pipelineComponents.PipelineRunsPage"

frontend/packages/pipelines-plugin/src/plugin.tsx
@@ -49,14 +28,7 @@ const plugin: Plugin<ConsumedExtensions> = [
{
type: 'ModelDefinition',
properties: {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can move all the ModelDefinition entries to dynamic plugin as well.

Copy link
Contributor Author

@debsmita1 debsmita1 Aug 25, 2021 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added the dynamic extension for model definition in the console-extention.json file but have kept this as is because the dynamic ex for this is not been consumed yet

frontend/packages/pipelines-plugin/src/topology/pipelinesTopologyPlugin.ts Outdated
@@ -31,16 +29,4 @@ export const pipelinesTopologyPlugin: Plugin<PipelineTopologyConsumedExtensions>
required: [FLAG_OPENSHIFT_PIPELINE],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should move this type: 'Topology/DataModelFactory' to dynamic plugin as well and remove this PipelineTopologyPlugin from plugin.tsx .

frontend/packages/pipelines-plugin/src/plugin.tsx Outdated
)
).default,
},
},
...pipelinesTopologyPlugin,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can remove this, based on my previous comment.

frontend/packages/pipelines-plugin/src/plugin.tsx Outdated
type ConsumedExtensions =
| ModelDefinition
| ModelFeatureFlag
| ResourceListPage
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we moved most of this to dynamic we can remove this from this file.

@openshift-ci openshift-ci bot added the kind/i18n Indicates issue or PR relates to internationalization or has content that needs to be translated label Aug 25, 2021
rohitkrai03
rohitkrai03 suggested changes Aug 25, 2021
View reviewed changes
frontend/packages/pipelines-plugin/package.json Outdated
@@ -24,8 +24,7 @@
"yamlTemplates": "src/templates/pipelines.ts",
"catalog": "src/components/catalog",
"topology": "src/topology",
"pipelineTabbedPage": "src/components/pipelines/PipelineTabbedPage.tsx",
"repositoryListComponent": "src/components/repository/list-page"
"pipelineComponents": "src/components"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's a good idea to separate exposed modules for separate areas to keep the chunk size smaller. I saw an earlier comment from @karthikjeeyar to merge it together into one module. This will increase the chunk size and load up everything at once. Maybe do not create so many modules. Try to group things together which you think should load together.

frontend/packages/pipelines-plugin/src/plugin.tsx Outdated
@@ -69,310 +20,6 @@ const plugin: Plugin<ConsumedExtensions> = [
).then((m) => m.default),
},
},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if the overview section extension is being used anymore with the new topology sidebar extensions. @debsmita1 Can check if this is still being used?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed it

@karthikjeeyar
Copy link
Contributor

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Aug 26, 2021
rohitkrai03
rohitkrai03 approved these changes Aug 26, 2021
View reviewed changes
Copy link
Contributor

@rohitkrai03 rohitkrai03 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/approve

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Aug 26, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: debsmita1, karthikjeeyar, 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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 26, 2021
@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@karthikjeeyar
Copy link
Contributor

/label px-approved
/label docs-approved
/label qe-approved

@openshift-ci openshift-ci bot added px-approved Signifies that Product Support has signed off on this PR docs-approved Signifies that Docs has signed off on this PR qe-approved Signifies that QE has signed off on this PR labels Aug 27, 2021
@openshift-merge-robot openshift-merge-robot merged commit 0de25d3 into openshift:master Aug 27, 2021
@spadgett spadgett added this to the v4.9 milestone Aug 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rohitkrai03 rohitkrai03 rohitkrai03 approved these changes

@abhinandan13jan abhinandan13jan Awaiting requested review from abhinandan13jan

@divyanshiGupta divyanshiGupta Awaiting requested review from divyanshiGupta

@karthikjeeyar karthikjeeyar Awaiting requested review from karthikjeeyar

Assignees

@rohitkrai03 rohitkrai03

@karthikjeeyar karthikjeeyar

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. component/pipelines Related to pipelines-plugin docs-approved Signifies that Docs has signed off on this PR kind/i18n Indicates issue or PR relates to internationalization or has content that needs to be translated lgtm Indicates that a PR is ready to be merged. px-approved Signifies that Product Support has signed off on this PR qe-approved Signifies that QE has signed off on this PR
Projects
None yet
Milestone
v4.9
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@debsmita1 @karthikjeeyar @openshift-bot @rohitkrai03 @spadgett @openshift-merge-robot