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.

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

Fetch knative resources and updated sidebar resources for Overview via extension #1800

Conversation

invincibleJai
Copy link
Member

@invincibleJai invincibleJai commented Jun 24, 2019

Fetch knative resources and updated sidebar resources

  • Fetch resources for knative
  • Show knative routes in topology view as a decorator
  • Show Revisions, Routes and Configurations in side bar under resources
  • Update id's in order to make it unique as kind is same in case of route and service Jira #Console-1550

Jira link

Gif

ezgif com-video-to-gif (3)

Overview

Added extension to fetch resources for CRDs and handled Resource tab update across.

Screenshot 2019-07-05 at 1 48 42 PM

@openshift-ci-robot
Copy link
Contributor

Hi @invincibleJai. Thanks for your PR.

I'm waiting for a openshift member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

@openshift-ci-robot openshift-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jun 24, 2019
@invincibleJai
Copy link
Member Author

/ok-to-test

@openshift-ci-robot
Copy link
Contributor

@invincibleJai: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

/ok-to-test

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.

@spadgett spadgett added this to the v4.2 milestone Jun 24, 2019
@christianvogt
Copy link
Contributor

@invincibleJai A general comment is we want to be able to contain our extensions to console in the dev console plugin. Quite possibly creating a knative plugin in the future. Often changes will be required to be able to extend an existing component in which case you should try to make the component extensible without leaking the dev console or knative domain logic into the base components.

Since the resource details component is shared and we want the revisions to show up in all instances of the overview component, we'll need a plugin extension for contributing to this component.

@@ -91,6 +98,30 @@ const TopologyDataController: React.FC<TopologyDataControllerProps> = ({
namespace,
prop: 'buildconfigs',
},
{
isList: true,
kind: referenceForModel(RevisionModel),
Copy link
Contributor

Choose a reason for hiding this comment

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

What happens to topology when knative CRDs aren't available?

Copy link
Member

Choose a reason for hiding this comment

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

What happens to topology when knative CRDs aren't available?

+1, this needs a feature flag based on the presence of the CRDs. Then we only try to watch resources that exist. Otherwise it's an error.

*/
private getKSRoute(dc: ResourceProps): ResourceProps {
// get the knative route
const IS_KNATIVE = 'serving.knative.dev/service';
Copy link
Contributor

Choose a reason for hiding this comment

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

move constant to top of file

@@ -889,3 +889,54 @@ export const InfrastructureModel: K8sKind = {
crd: true,
};

export const RevisionModel: K8sKind = {
Copy link
Contributor

Choose a reason for hiding this comment

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

If we can plug into the console for knative, then these models should be defined in dev-console package.

<BuildOverview buildConfigs={buildConfigs} />
<NetworkingOverview services={services} routes={routes} />
</React.Fragment>
) : <KnativeOverview ksroutes={ksroutes} configurations={configurations} revisions={revisions} />}
Copy link
Contributor

Choose a reason for hiding this comment

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

We need to find a way to plug into the console without adding knative specific branching logic.

@invincibleJai
Copy link
Member Author

/ok-to-test

@openshift-ci-robot openshift-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jun 25, 2019
@christianvogt
Copy link
Contributor

@vojtechszocs does kubevirt need to extend the resource details panel?
I think we need an extension for this PR so that knative support doesn't get hard coded into these views.

fyi @spadgett

@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 25, 2019
@openshift-ci-robot openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 26, 2019
@invincibleJai
Copy link
Member Author

/retest

@invincibleJai invincibleJai force-pushed the fix-knative-resource-overview branch 2 times, most recently from e1cc4a5 to d9880c9 Compare June 26, 2019 18:13
@christianvogt
Copy link
Contributor

/hold
until we look at extensibility

@openshift-ci-robot openshift-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jun 26, 2019
@invincibleJai invincibleJai force-pushed the fix-knative-resource-overview branch from d9880c9 to bc5107e Compare July 2, 2019 03:18
@openshift-ci-robot openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 2, 2019
@invincibleJai invincibleJai force-pushed the fix-knative-resource-overview branch 3 times, most recently from 7e7b829 to 7dc29f6 Compare July 2, 2019 09:37
@invincibleJai
Copy link
Member Author

/test e2e-aws

@invincibleJai invincibleJai changed the title fix(knative): fetch knative resources and updated sidebar resources Fetch knative resources and updated sidebar resources for Overview via extension Jul 16, 2019
@invincibleJai
Copy link
Member Author

/test e2e-aws-console-olm

@invincibleJai
Copy link
Member Author

/test e2e-aws

@invincibleJai
Copy link
Member Author

@christianvogt @vojtechszocs have handled review comments, pls take a look and let me know.

Thanks :)

@@ -9,3 +9,4 @@ export * from './nav-items';
export * from './pages';
export * from './perspectives';
export * from './yaml-templates';
export * from './overview';
Copy link
Contributor

Choose a reason for hiding this comment

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

please put in alphabetical order

@@ -8,10 +8,10 @@ import {
} from '@console/internal/models';
import { k8sCreate, K8sResourceKind } from '@console/internal/module/k8s';
import { SelectorInput } from '@console/internal/components/utils';
import { createKnativeService } from '@console/knative-plugin/src/utils/create-knative-utils';
Copy link
Contributor

Choose a reason for hiding this comment

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

Why move this? It just adds one more thing to be reviewed.

Copy link
Member Author

Choose a reason for hiding this comment

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

had discussed with @christianvogt and IMO it would make more sense to add knative specific utils in knative-plugin as we have now

Copy link
Contributor

Choose a reason for hiding this comment

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

ok

@invincibleJai invincibleJai force-pushed the fix-knative-resource-overview branch 2 times, most recently from 826dcab to 280241c Compare July 16, 2019 17:38
@joshuawilson
Copy link
Contributor

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 16, 2019
@christianvogt
Copy link
Contributor

/approve

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 16, 2019
@openshift-bot
Copy link
Contributor

/retest

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

@joshuawilson
Copy link
Contributor

/test e2e-aws

@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Jul 17, 2019
@joshuawilson
Copy link
Contributor

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 17, 2019
@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: christianvogt, invincibleJai, joshuawilson

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

@invincibleJai
Copy link
Member Author

/test e2e-aws-console-olm

@gijohn
Copy link
Contributor

gijohn commented Jul 17, 2019

/test e2e-aws

1 similar comment
@gijohn
Copy link
Contributor

gijohn commented Jul 17, 2019

/test e2e-aws

@joshuawilson
Copy link
Contributor

/retest

@openshift-merge-robot openshift-merge-robot merged commit 6970d8f into openshift:master Jul 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants