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

filter deployments created by event sources #4770

Merged
merged 1 commit into from Mar 27, 2020
Merged

filter deployments created by event sources #4770

merged 1 commit into from Mar 27, 2020

Conversation

nemesis09
Copy link
Contributor

@nemesis09 nemesis09 commented Mar 19, 2020 •
edited

Fixes:
https://issues.redhat.com/browse/ODC-3011

Analysis / Root cause:
Some Event Sources have associated deployment which are shown in topology view along with the eventSource e.g in case of kafka and api server source.

Solution Description:
Topology view should only show eventSource and not the deployments associated with it

Screens:
filtereventdeployment

Test Coverage:
Screenshot from 2020-03-19 08-24-25

Browser Conformance:

  • Firefox
  • Chrome
  • Safari
  • Edge

@openshift-ci-robot openshift-ci-robot added component/dev-console Related to dev-console component/knative Related to knative-plugin labels Mar 19, 2020
@nemesis09
Copy link
Contributor Author

/retest

invincibleJai
invincibleJai reviewed Mar 19, 2020
View reviewed changes
frontend/packages/knative-plugin/src/utils/knative-topology-utils.ts Outdated
Comment on lines 433 to 440
const allEventSourceModels: K8sKind[] = [
EventSourceCronJobModel,
EventSourceContainerModel,
EventSourceApiServerModel,
EventSourceCamelModel,
EventSourceKafkaModel,
EventSourceServiceBindingModel,
];
Copy link
Member

Choose a reason for hiding this comment

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

we need to see if there is a way to handle it in a generic way i.e without have check for specific models for ES as dynamic sources will in introduced soon. Then this may not work

Copy link
Contributor

Choose a reason for hiding this comment

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

We still don't have an solution for dynamic event sources, so dealing with the static set is ok for now.

Copy link
Member

Choose a reason for hiding this comment

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

i suggested to create a util which will roll up uid's for eventsource and then check for it against ownerefrences in deployments if they match filter it out, I think this should do what we want instead of using all ES models.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have used the knEventSources which was already present in the scope, passed it to the filterNonKnativeDeployments function as an optional argument to use as reference to filter the deployments for event sources.

@andrewballantyne
Copy link
Contributor

/kind feature

@openshift-ci-robot openshift-ci-robot added the kind/feature Categorizes issue or PR as related to a new feature. label Mar 20, 2020
@nemesis09 nemesis09 force-pushed the filter-event-deployment branch 3 times, most recently from 4d52ebb to f52c588 Compare March 24, 2020 00:17
@nemesis09
Copy link
Contributor Author

/retest

2 similar comments
@nemesis09
Copy link
Contributor Author

/retest

@invincibleJai
Copy link
Member

/retest

@invincibleJai
Copy link
Member

@nemesis09 pls rebase with master

sahil143
sahil143 reviewed Mar 24, 2020
View reviewed changes
frontend/packages/knative-plugin/src/utils/__tests__/knative-topology-utils.spec.ts Outdated
sampleEventSourceApiServer.data[0],
]);
expect(filteredResources).toHaveLength(1);
expect(filteredResources[0].metadata?.name).toEqual('analytics-deployment');
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Is there a chance that mock data will not have metadata.name?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't find any such event source yet. although metadata itself is optional.

Copy link
Member

Choose a reason for hiding this comment

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

@nemesis09 i think what @sahil143 mean is these are mock data so it'll always have metadata.name

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This test checks of the deployments for knative service and event sources have been filtered out, leaving behind only the deployment which was not associated with either knative service or event source, i.e analytics-deployment

frontend/packages/knative-plugin/src/utils/knative-topology-utils.ts Outdated
const KNATIVE_EVENTS_CAMEL = 'sources.eventing.knative.dev/camelSource';
const KNATIVE_EVENTS_KAFKA = 'sources.eventing.knative.dev/kafkaSource';
const isEventSourceKind = (uid: string): boolean =>
!!eventSources?.find((eventSource) => eventSource.metadata?.uid === uid);
return _.filter(resources, (d) => {
return (
!_.get(d, ['metadata', 'labels', KNATIVE_CONFIGURATION]) &&
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: third param for _.get

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

frontend/packages/knative-plugin/src/utils/knative-topology-utils.ts Outdated
!_.get(d, ['metadata', 'labels', KNATIVE_EVENTS_APISERVER]) &&
!_.get(d, ['metadata', 'labels', KNATIVE_EVENTS_CAMEL]) &&
!_.get(d, ['metadata', 'labels', KNATIVE_EVENTS_KAFKA])
!isEventSourceKind(d.metadata?.ownerReferences ? d.metadata.ownerReferences[0].uid : '')
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
!isEventSourceKind(d.metadata?.ownerReferences ? d.metadata.ownerReferences[0].uid : '')
!isEventSourceKind(d.metadata?.ownerReferences?.[0].uid)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

frontend/packages/knative-plugin/src/utils/knative-topology-utils.ts Outdated
const KNATIVE_EVENTS_CAMEL = 'sources.eventing.knative.dev/camelSource';
const KNATIVE_EVENTS_KAFKA = 'sources.eventing.knative.dev/kafkaSource';
const isEventSourceKind = (uid: string): boolean =>
!!eventSources?.find((eventSource) => eventSource.metadata?.uid === uid);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
!!eventSources?.find((eventSource) => eventSource.metadata?.uid === uid);
uid && !!eventSources?.find((eventSource) => eventSource.metadata?.uid === uid);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@nemesis09 nemesis09 force-pushed the filter-event-deployment branch 4 times, most recently from 3816894 to 4201a89 Compare March 27, 2020 04:40
@invincibleJai
Copy link
Member

/approve

Verified locally works as expected

sahil143
sahil143 reviewed Mar 27, 2020
View reviewed changes
Copy link
Contributor

@sahil143 sahil143 left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: invincibleJai, nemesis09, sahil143

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-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 27, 2020
@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 27, 2020
@openshift-bot
Copy link
Contributor

/retest

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

@openshift-bot
Copy link
Contributor

/retest

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

@openshift-merge-robot openshift-merge-robot merged commit 8df9827 into openshift:master Mar 27, 2020
@spadgett spadgett added this to the v4.5 milestone Mar 31, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@invincibleJai invincibleJai invincibleJai left review comments

@sahil143 sahil143 sahil143 left review comments

@christianvogt christianvogt christianvogt left review comments

@jeff-phillips-18 jeff-phillips-18 Awaiting requested review from jeff-phillips-18

Assignees

@sahil143 sahil143

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. component/dev-console Related to dev-console component/knative Related to knative-plugin kind/feature Categorizes issue or PR as related to a new feature. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Milestone
v4.5
Development

Successfully merging this pull request may close these issues.

None yet
9 participants
@nemesis09 @andrewballantyne @invincibleJai @openshift-ci-robot @openshift-bot @sahil143 @christianvogt @spadgett @openshift-merge-robot