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

Add Knative event sources component #6175

Merged
merged 2 commits into from
Nov 5, 2019
Merged

Add Knative event sources component #6175

merged 2 commits into from
Nov 5, 2019

Conversation

antoineco
Copy link
Contributor

@antoineco antoineco commented Oct 30, 2019

Description

Changes proposed in this pull request:

  • Add a controller for the reconciliation of custom Knative event sources (HTTPSource only for the time being)

Related issue(s)

#6033 (Preparation work)
#6150

@CLAassistant
Copy link

CLAassistant commented Oct 30, 2019

CLA assistant check
All committers have signed the CLA.

@antoineco antoineco requested review from k15r, marcobebway and nachtmaar and removed request for marcobebway October 30, 2019 11:33
@netlify
Copy link

netlify bot commented Oct 30, 2019

🥰 Documentation preview ready! 🥰

Built with commit 274e251

https://deploy-preview-6175--kyma-project-docs-preview.netlify.com

Copy link
Contributor

@nachtmaar nachtmaar left a comment

Choose a reason for hiding this comment

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

Controller looks really good to me 👍

components/event-sources/LICENSE Outdated Show resolved Hide resolved
components/event-sources/Makefile Show resolved Hide resolved
components/event-sources/README.md Outdated Show resolved Hide resolved
components/event-sources/cmd/sources-controller/main.go Outdated Show resolved Hide resolved
components/event-sources/config/100-namespace.yaml Outdated Show resolved Hide resolved
@antoineco antoineco requested a review from devguyio as a code owner November 4, 2019 16:14
{
Name: "Invalid object key",
Key: tNs + "/" + tName + "/invalid",
WantErr: true,
Copy link
Contributor

Choose a reason for hiding this comment

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

are we able to specify which errors to expect in such case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately not, we would have to open a PR to add that feature to Knative's table tests: https://godoc.org/knative.dev/pkg/reconciler/testing#TableRow

Copy link

@piotrmsc piotrmsc left a comment

Choose a reason for hiding this comment

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

Please add CODEOWNERS for this component. Question, can this component be outside kyma mono repo? (Is it kyma specific?)

@devguyio
Copy link
Contributor

devguyio commented Nov 5, 2019

@piotrmsc IMHO this should stay in Kyma as Application Operator, and Application Broker depend on it. This is synonym to the Event Service.

@antoineco
Copy link
Contributor Author

@piotrmsc done, thanks for pointing that out.
To your question, yes this component is Kyma specific, the application controller will use it to create event sources in the near future (see the epic this issue is in).

@piotrmsc
Copy link

piotrmsc commented Nov 5, 2019

@piotrmsc done, thanks for pointing that out.
To your question, yes this component is Kyma specific, the application controller will use it to create event sources in the near future (see the epic this issue is in).

Ok, in such case it should stay in kyma.

}

// makeKnService returns the desired Knative Service object for a given
// HTTPSource. An optional Knative Service can be passed as parameter, in which
Copy link
Contributor

Choose a reason for hiding this comment

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

the comment should somehow reflect that only 0 or 1 knative services can be passed in the variadic currrentKsvc.

Copy link
Contributor Author

@antoineco antoineco Nov 5, 2019

Choose a reason for hiding this comment

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

Isn't it clear from the phrase "An optional Knative Service can be passed as parameter"? It's singular, and there is the word optional. For an internal method that will do IMO.

Copy link
Contributor

@devguyio devguyio left a comment

Choose a reason for hiding this comment

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

Left a couple of non essential questions, but it lgtm

components/event-sources/README.md Show resolved Hide resolved
components/event-sources/config/200-rbac.yaml Show resolved Hide resolved
Copy link
Contributor

@bszwarc bszwarc left a comment

Choose a reason for hiding this comment

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

Added some comments.

components/event-sources/README.md Outdated Show resolved Hide resolved
components/event-sources/README.md Show resolved Hide resolved
components/event-sources/README.md Outdated Show resolved Hide resolved
components/event-sources/README.md Show resolved Hide resolved
components/event-sources/README.md Outdated Show resolved Hide resolved
components/event-sources/README.md Outdated Show resolved Hide resolved
components/event-sources/README.md Outdated Show resolved Hide resolved
components/event-sources/README.md Outdated Show resolved Hide resolved
components/event-sources/README.md Outdated Show resolved Hide resolved
components/event-sources/README.md Outdated Show resolved Hide resolved
Copy link
Contributor

@bszwarc bszwarc left a comment

Choose a reason for hiding this comment

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

Added some comments.

antoineco and others added 2 commits November 5, 2019 19:12
Includes HTTPSource type and corresponding controller.

Co-Author: Marco Bebway <marco.bebway@sap.com>
Co-Author: Ahmed Hamouda <a.hamouda@sap.com>
Co-Authored-By: Barbara Szwarc <barbara.m.szwarc@gmail.com>
@antoineco antoineco merged commit 38a6ad7 into kyma-project:master Nov 5, 2019
@antoineco antoineco deleted the event-source-ctrler branch November 5, 2019 18:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/eventing Issues or PRs related to eventing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants