Skip to content

adds support for pingSource #5015

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
Merged
openshift-merge-robot merged 1 commit into from
Apr 18, 2020
Merged

adds support for pingSource #5015

openshift-merge-robot merged 1 commit into from
Apr 18, 2020

Conversation

@invincibleJai
Copy link
Member

@invincibleJai invincibleJai commented Apr 13, 2020 •
edited
Loading

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

Analysis / Root cause:
Currently PingSource is not supported as known sources

Solution Description:
Adds support for PingSource with Form in add flow

Screen shots / Gifs for design review:
Apr-13-2020 13-08-14

@serenamarie125

Unit test coverage report:
Screenshot 2020-04-13 at 1 01 02 PM
Screenshot 2020-04-13 at 1 03 39 PM

Browser conformance:

  • Chrome
  • Firefox
  • Safari
  • Edge

@openshift-ci-robot openshift-ci-robot added the component/dev-console Related to dev-console label Apr 13, 2020
@openshift-ci-robot openshift-ci-robot added component/knative Related to knative-plugin approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Apr 13, 2020
@invincibleJai
Copy link
Member Author

invincibleJai commented Apr 13, 2020

/kind feature

@openshift-ci-robot openshift-ci-robot added the kind/feature Categorizes issue or PR as related to a new feature. label Apr 13, 2020
@invincibleJai
Copy link
Member Author

invincibleJai commented Apr 13, 2020

/assign @rohitkrai03

rohitkrai03
rohitkrai03 suggested changes Apr 13, 2020
View reviewed changes
sahil143
sahil143 reviewed Apr 13, 2020
View reviewed changes
frontend/packages/dev-console/src/components/topology/topology-types.ts Outdated
Copy link
Contributor

@sahil143 sahil143 Apr 13, 2020

Choose a reason for hiding this comment

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

@invincibleJai it would be good if we started using referenceForModel(PingsourceModel). With my topology PR these key wont exist anymore.
I will change the other keys in my PR to use the same.

frontend/packages/knative-plugin/src/components/add/import-types.ts Outdated
Comment on lines 3 to 11
Copy link
Contributor

@sahil143 sahil143 Apr 13, 2020

Choose a reason for hiding this comment

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

Are values in this enum kind of the respective EventSource? If yes, we should be using their respective Model.kind here.

Copy link
Member Author

@invincibleJai invincibleJai Apr 14, 2020

Choose a reason for hiding this comment

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

yes, but then may have to use normal Object as in enum's computed values are not permitted in an enum with string valued members

Copy link
Member Author

@invincibleJai invincibleJai Apr 14, 2020

Choose a reason for hiding this comment

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

have made it a const and used model.kind

frontend/packages/knative-plugin/src/topology/__tests__/topology-knative-test-data.ts Outdated
Copy link
Contributor

@sahil143 sahil143 Apr 13, 2020

Choose a reason for hiding this comment

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

same here, we should use referenceForModel(PingsourceModel) as key here.

frontend/packages/knative-plugin/src/utils/get-knative-icon.ts Outdated
Comment on lines 26 to 25
Copy link
Contributor

@sahil143 sahil143 Apr 13, 2020

Choose a reason for hiding this comment

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

same Icon for both of them?

Copy link
Member Author

@invincibleJai invincibleJai Apr 14, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

CronJob is deprecated with PingSource so used same as CronJob should go away with next release of Eventing Operator so used the same , can even go with default icon.

Copy link
Member Author

@invincibleJai invincibleJai Apr 14, 2020

Choose a reason for hiding this comment

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

updated with Default icon

@invincibleJai invincibleJai force-pushed the feat-ping-src branch 4 times, most recently from 9a2fe56 to a350b1f Compare April 14, 2020 08:04
@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 16, 2020
@openshift-ci-robot openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 16, 2020
serenamarie125
serenamarie125 approved these changes Apr 17, 2020
View reviewed changes
Copy link
Contributor

@serenamarie125 serenamarie125 left a comment

Choose a reason for hiding this comment

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

LGTM @invincibleJai ! Approved by UX

@invincibleJai invincibleJai force-pushed the feat-ping-src branch 5 times, most recently from eb5c706 to de0017a Compare April 17, 2020 10:01
sahil143
sahil143 suggested changes Apr 17, 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

verified changes locally

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 17, 2020
@invincibleJai
Copy link
Member Author

invincibleJai commented Apr 17, 2020

/retest

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

sahil143 commented Apr 17, 2020

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 17, 2020
@invincibleJai
Copy link
Member Author

invincibleJai commented Apr 17, 2020

/retest

1 similar comment
@invincibleJai
Copy link
Member Author

invincibleJai commented Apr 17, 2020

/retest

@rohitkrai03
Copy link
Contributor

rohitkrai03 commented Apr 17, 2020

/lgtm

@openshift-ci-robot
Copy link
Contributor

openshift-ci-robot commented Apr 17, 2020

[APPROVALNOTIFIER] This PR is APPROVED

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

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details 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

invincibleJai commented Apr 17, 2020

/retest

1 similar comment
@nemesis09
Copy link
Contributor

nemesis09 commented Apr 17, 2020

/retest

@nemesis09
Copy link
Contributor

nemesis09 commented Apr 17, 2020

/test e2e-gcp-console

@spadgett
Copy link
Member

spadgett commented Apr 17, 2020

/hold
for #5100

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 17, 2020
@spadgett
Copy link
Member

spadgett commented Apr 17, 2020

/hold cancel
/retest

@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 17, 2020
@openshift-bot
Copy link
Contributor

openshift-bot commented Apr 18, 2020

/retest

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

@openshift-merge-robot openshift-merge-robot merged commit e613c59 into openshift:master Apr 18, 2020
@spadgett spadgett added this to the v4.5 milestone Apr 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@christianvogt christianvogt Awaiting requested review from christianvogt

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

3 more reviewers

@rohitkrai03 rohitkrai03 rohitkrai03 requested changes

@sahil143 sahil143 sahil143 approved these changes

@serenamarie125 serenamarie125 serenamarie125 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@rohitkrai03 rohitkrai03

@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.

9 participants

@invincibleJai @sahil143 @rohitkrai03 @openshift-ci-robot @nemesis09 @spadgett @openshift-bot @serenamarie125 @openshift-merge-robot