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

adds support for apiServerSource #4810

Merged

Conversation

invincibleJai
Copy link
Member

@invincibleJai invincibleJai commented Mar 24, 2020

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

Analysis / Root cause:
User can't create ApiServerSource via web console

Solution Description:
Adds support for ApiServerSource via web console

Screen shots / Gifs for design review:
Screenshot 2020-03-27 at 1 41 04 PM

Unit test coverage report:
Screenshot 2020-03-25 at 1 16 38 AM

Browser conformance:

  • Chrome
  • Firefox
  • Safari
  • Edge

cc @serenamarie125

@openshift-ci-robot openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 24, 2020
@openshift-ci-robot openshift-ci-robot added component/knative Related to knative-plugin component/shared Related to console-shared labels Mar 24, 2020
@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 24, 2020
@invincibleJai invincibleJai force-pushed the feat-apiserversource branch 3 times, most recently from f5240b7 to 925196b Compare March 26, 2020 06:46
@invincibleJai invincibleJai changed the title [WIP]adds support for apiServerSource adds support for apiServerSource Mar 26, 2020
@openshift-ci-robot openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 26, 2020
@divyanshiGupta
Copy link
Contributor

divyanshiGupta commented Mar 26, 2020

Screenshot from 2020-03-26 19-42-21

@invincibleJai I am getting this error while trying to create an EventSource of type ApiServerSource. Tried to use the same data for testing as you have shown in the screenshot. Am I missing anything?

@invincibleJai
Copy link
Member Author

@divyanshiGupta yeah it's my bad should have given proper values for mode we need Resource

you can follow https://docs.google.com/document/d/1F4zECoLRvaUiigLaogbgYOiD_hB_w5hAtcXGY0MxVc4/edit

@invincibleJai invincibleJai force-pushed the feat-apiserversource branch 2 times, most recently from 9274fa9 to e9da1cc Compare March 27, 2020 08:11
@invincibleJai
Copy link
Member Author

have updated mode to Dropdown as currently it just takes two values and defaults to Reference

cc @divyanshiGupta @sahil143

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.

Screenshot from 2020-03-27 14-23-37
@invincibleJai typeahead is not working for service account name.

)
.required('Required'),
mode: yup.string().max(253, 'Cannot be longer than 253 characters.'),
serviceAccountName: yup.string().max(253, 'Cannot be longer than 253 characters.'),
Copy link
Contributor

Choose a reason for hiding this comment

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

ServiceAccountName is a dropdown field. Do we still need this?

Copy link
Contributor

Choose a reason for hiding this comment

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

Mode is also a dropdown field now. @invincibleJai I think the two validations can be removed.

Copy link
Member Author

Choose a reason for hiding this comment

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

yes removed both :)

@@ -60,7 +60,7 @@ const EventSinkServicesOverviewList: React.FC<EventSinkServicesOverviewListProps
) : (
<span className="text-muted">No services found for this resource.</span>
)}
{pods?.length && <PodsOverview pods={pods} obj={obj} allPodsLink={linkUrl} />}
{pods?.length > 0 && <PodsOverview pods={pods} obj={obj} allPodsLink={linkUrl} />}
Copy link
Contributor

Choose a reason for hiding this comment

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

If pods length is 0, don't we want to show empty msg? No pods found for this resource

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah some sources may not have Pods or associated deployments so had this check

@invincibleJai invincibleJai force-pushed the feat-apiserversource branch 2 times, most recently from 597efb8 to 89b776b Compare March 27, 2020 09:35
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 Mar 27, 2020
@sahil143
Copy link
Contributor

/lgtm cancel

There need to be some changes in the Mode dropdown. @invincibleJai is looking into it

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

/lgtm cancel

There need to be some changes in the Mode dropdown. @invincibleJai is looking into it

Change was wrt mode value and it's Ref have updated , PTAL

https://github.com/knative/eventing/blob/6b9f75aaf425cbdef451ec069f0eab20371d0990/pkg/apis/sources/v1alpha1/apiserver_validation.go#L27

@divyanshiGupta
Copy link
Contributor

Code looks good. Tested locally, was able to create ApiServerSource.
/lgtm

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

/lgtm

Mode dropdown changes works fine. Verified changes locally.

@serenamarie125
Copy link
Contributor

General comment @invincibleJai - we will not have final text available for labels/help text at this time, so please don't worry about updating those now. We will have a follow up design story to work on that.

@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 28, 2020
@openshift-ci-robot openshift-ci-robot removed lgtm Indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Mar 30, 2020
@openshift-ci-robot openshift-ci-robot added the component/core Related to console core functionality label Mar 30, 2020
@invincibleJai
Copy link
Member Author

invincibleJai commented Mar 30, 2020

@spadgett @benjaminapetersen @christianvogt needed your help with review of 0b76e21

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.

@invincibleJai We should look to add StickyFooter for event sources form as well. Probably need to confirm with UX and can be done in a separate PR.

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

@sahil143 have added sticky :)

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 openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 2, 2020
@christianvogt
Copy link
Contributor

/approve

@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: christianvogt, divyanshiGupta, invincibleJai, 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 Apr 2, 2020
@invincibleJai
Copy link
Member Author

/retest

@openshift-merge-robot openshift-merge-robot merged commit 229af0a into openshift:master Apr 3, 2020
@spadgett spadgett added this to the v4.5 milestone Apr 7, 2020
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. component/core Related to console core functionality component/knative Related to knative-plugin component/shared Related to console-shared 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
Development

Successfully merging this pull request may close these issues.

None yet

9 participants