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

ODC-7377: Add Functions tab to the side navigation menu in Developer prespective #13138

Closed

Conversation

lokanandaprabhu
Copy link
Contributor

@lokanandaprabhu lokanandaprabhu commented Sep 7, 2023

Story:
https://issues.redhat.com/browse/ODC-7377

Description:

In the developer perspective, in the left side navigation menu, added Functions tab inside Resources section, which list down all the serverless functions created for the specific namepsace and on click of function it opens the Service details tab

Screenshots:

Screen.Recording.2023-09-07.at.12.41.59.PM.mov

Tests:
Screenshot 2023-09-08 at 1 02 59 PM

@openshift-ci-robot openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Sep 7, 2023
@openshift-ci-robot
Copy link
Contributor

openshift-ci-robot commented Sep 7, 2023

@lokanandaprabhu: This pull request references ODC-7377 which is a valid jira issue.

In response to this:

Story:
https://issues.redhat.com/browse/ODC-7377

Description:

In the developer perspective, in the left side navigation menu, added Functions tab inside Resources section, which list down all the serverless functions created for the specific namepsace and on click of function it opens the Service details tab

Screenshots:

Screen.Recording.2023-09-07.at.12.41.59.PM.mov

Tests:
To-Do

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 openshift-ci bot added the component/dev-console Related to dev-console label Sep 7, 2023
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Sep 7, 2023

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: lokanandaprabhu
Once this PR has been reviewed and has the lgtm label, please assign invinciblejai for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found 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 component/knative Related to knative-plugin component/shared Related to console-shared kind/i18n Indicates issue or PR relates to internationalization or has content that needs to be translated labels Sep 7, 2023
@lokanandaprabhu lokanandaprabhu force-pushed the feature/ODC-7377 branch 2 times, most recently from 2ed88e7 to fb94b99 Compare September 8, 2023 09:18
@openshift-ci-robot
Copy link
Contributor

openshift-ci-robot commented Sep 8, 2023

@lokanandaprabhu: This pull request references ODC-7377 which is a valid jira issue.

In response to this:

Story:
https://issues.redhat.com/browse/ODC-7377

Description:

In the developer perspective, in the left side navigation menu, added Functions tab inside Resources section, which list down all the serverless functions created for the specific namepsace and on click of function it opens the Service details tab

Screenshots:

Screen.Recording.2023-09-07.at.12.41.59.PM.mov

Tests:
Screenshot 2023-09-08 at 1 02 59 PM

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.

@lokanandaprabhu
Copy link
Contributor Author

/cc @jerolimov

Copy link
Member

@jerolimov jerolimov left a comment

Choose a reason for hiding this comment

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

It works already really well and the code looks almost good to me. 👍

Just some minor ideas... (see below)

@sanketpathak can you please take a look into the e2e tests?

/uncc jcaianirh divyanshiGupta
/cc @sanketpathak @vikram-raj

Comment on lines +32 to +44
const [revisions, revisionLoaded, revisionErrorLoad] = useK8sWatchResource<RevisionKind[]>({
kind: referenceForModel(RevisionModel),
namespace: obj.metadata.namespace,
isList: true,
});

const revisionsForService =
revisionLoaded &&
!revisionErrorLoad &&
revisions.filter(
(revision) =>
_.get(revision.metadata, `labels["serving.knative.dev/service"]`) === obj.metadata.name,
);
Copy link
Member

Choose a reason for hiding this comment

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

I think you can use a labelSelector in useK8sWatchResource to get only the matching revisions instead of loading all Revisions and then filter them on the client.

Comment on lines +57 to +81
<div className="col-md-6">
{isServerlessFunction(obj) && (
<DetailsItem label={t('knative-plugin~Type')} obj={obj}>
{t('knative-plugin~Function')}
</DetailsItem>
)}
{obj?.status?.url && (
<DetailsItem label={t('knative-plugin~URL')} obj={obj} path="status.url">
<ExternalLink
href={obj.status.url}
additionalClassName="co-external-link--block"
text={obj.status.url}
/>
</DetailsItem>
)}
{revisionsForService && revisionLoaded && !revisionErrorLoad && (
<DetailsItem label={t('knative-plugin~Revisions')} obj={obj} path="status.traffic">
<RevisionsOverviewList
revisions={revisionsForService}
service={obj}
hideSectionHeading
/>
</DetailsItem>
)}
</div>
Copy link
Member

Choose a reason for hiding this comment

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

The DetailsItem component renders dt and dd, these HTML elements must be wrapped in a dl:

          <div className="col-md-6">
            <dl>
            {isServerlessFunction(obj) && (
              <DetailsItem label={t('knative-plugin~Type')} obj={obj}>
                {t('knative-plugin~Function')}
              </DetailsItem>
            )}
...

Comment on lines +201 to +205
export enum ServiceTypeValue {
Functions = 'Functions',
// eslint-disable-next-line @typescript-eslint/naming-convention
'Non-functions' = 'Non-functions',
}
Copy link
Member

Choose a reason for hiding this comment

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

It's an enum that should not be related to the labels that are displayed to the user. So you should clean this up:

Suggested change
export enum ServiceTypeValue {
Functions = 'Functions',
// eslint-disable-next-line @typescript-eslint/naming-convention
'Non-functions' = 'Non-functions',
}
export enum ServiceTypeValue {
Function = 'Function',
Service = 'Service',
}

If the value is what you render to the user, you should change that in that component.

}
},
"flags": {
"required": ["OPENSHIFT", "KNATIVE_SERVING", "KNATIVE_SERVING_SERVICE"]
Copy link
Member

Choose a reason for hiding this comment

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

I recently tried to remove all the OPENSHIFT flags as there is no dependency here to OPENSHIFT. It's a feature that works on all clusters with Knative.

Suggested change
"required": ["OPENSHIFT", "KNATIVE_SERVING", "KNATIVE_SERVING_SERVICE"]
"required": ["KNATIVE_SERVING", "KNATIVE_SERVING_SERVICE"]

"component": { "$codeRef": "overviewComponent.FunctionDetailsPage" }
},
"flags": {
"required": ["OPENSHIFT", "KNATIVE_SERVING", "KNATIVE_SERVING_SERVICE"]
Copy link
Member

Choose a reason for hiding this comment

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

Same as above. It's a feature that works on all clusters with Knative. Please remove OPENSHIFT from the dependency list:

Suggested change
"required": ["OPENSHIFT", "KNATIVE_SERVING", "KNATIVE_SERVING_SERVICE"]
"required": ["KNATIVE_SERVING", "KNATIVE_SERVING_SERVICE"]

@openshift-ci openshift-ci bot requested review from sanketpathak and vikram-raj and removed request for divyanshiGupta and jcaianirh September 20, 2023 16:19
@jerolimov
Copy link
Member

/retest

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Sep 20, 2023

@lokanandaprabhu: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/e2e-gcp-console 543fee4 link true /test e2e-gcp-console

Full PR test history. Your PR dashboard.

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. I understand the commands that are listed here.

@lokanandaprabhu
Copy link
Contributor Author

Addressed all review comments in PR - #13174

@lokanandaprabhu
Copy link
Contributor Author

Closing this PR since PR - #13174 handles this feature story as well.

/close

@openshift-ci openshift-ci bot closed this Sep 21, 2023
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Sep 21, 2023

@lokanandaprabhu: Closed this PR.

In response to this:

Closing this PR since PR - #13174 handles this feature story as well.

/close

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component/dev-console Related to dev-console component/knative Related to knative-plugin component/shared Related to console-shared jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. kind/i18n Indicates issue or PR relates to internationalization or has content that needs to be translated
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants