Skip to content

Update sample source guide #2475

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
merged 7 commits into from
May 27, 2020
Merged

Update sample source guide #2475

merged 7 commits into from
May 27, 2020

Conversation

@aliok
Copy link
Member

@aliok aliok commented May 18, 2020

Fixes #issue-number or description of the problem the PR solves

Proposed Changes

  • Validate the sample source guide
  • Fix the references, file names, etc.
  • Adaptions for SinkBinding changes Sink binding knative-extensions/sample-source#40
  • Create a separate page for instructions on how to make the source part of eventing-contrib

@googlebot googlebot added the cla: yes Indicates the PR's author has signed the CLA. label May 18, 2020
@aliok aliok changed the title Source guide Update sample source guide May 18, 2020
@knative-prow-robot knative-prow-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label May 18, 2020
aliok
aliok commented May 18, 2020
View reviewed changes
docs/eventing/samples/writing-receive-adapter-source/01-theory.md
Comment on lines -86 to -100
Ensure that the specific source subdirectory has been added to the injection portion of the `hack/update-codegen.sh` script.
```patch
# Sources
+API_DIRS_SOURCES=(github/pkg camel/source/pkg kafka/source/pkg awssqs/pkg couchdb/source/pkg prometheus/pkg YourSourceHere/pkg)
-API_DIRS_SOURCES=(github/pkg camel/source/pkg kafka/source/pkg awssqs/pkg couchdb/source/pkg prometheus/pkg)
```
and
```patch
-i knative.dev/eventing-contrib/camel/source/pkg/apis \
- -i knative.dev/eventing-contrib/github/pkg/apis
+ -i knative.dev/eventing-contrib/github/pkg/apis \
+ -i knative.dev/eventing-contrib/YourSourceHere/pkg/apis
Copy link
Member Author

@aliok aliok May 18, 2020

Choose a reason for hiding this comment

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

This part is now explained in a separate page: https://github.com/knative/docs/pull/2475/files#diff-7f8a6b25a6b73452947bc5e926d1e6c9R1

aliok
aliok commented May 18, 2020
View reviewed changes
docs/eventing/samples/writing-receive-adapter-source/05-receive-adapter.md
Comment on lines -70 to -80
## Reconcile/Create The Receive Adapter
As part of the source reconciliation, we have to create and deploy
(and update if necessary) the underlying receive adapter. The two
client sets used in this process is the `kubeClientSet` for the
Deployment tracking, and the `EventingClientSet` for the event
recording.
Verify the specified kubernetes resources are valid, and update the `Status` accordingly
Assemble the ReceiveAdapterArgs
Copy link
Member Author

@aliok aliok May 18, 2020

Choose a reason for hiding this comment

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

This section was actually duplicate of https://github.com/knative/docs/pull/2475/files#diff-df6286b13f3371b30a72bf3277b63a96R30

@aliok
Copy link
Member Author

aliok commented May 18, 2020

/assign @lberk
Mind reviewing?

@aliok aliok mentioned this pull request May 18, 2020
Closed
n3wscott
n3wscott reviewed May 18, 2020
View reviewed changes
Copy link
Contributor

@n3wscott n3wscott left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve

@knative-prow-robot knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label May 18, 2020
tom24d
tom24d reviewed May 18, 2020
View reviewed changes
Copy link
Contributor

@tom24d tom24d left a comment

Choose a reason for hiding this comment

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

Thoughts as new source contributor like me. This is my first review in my life so sorry if there is any inappropriate. Thanks!

@tom24d
Copy link
Contributor

tom24d commented May 18, 2020

Thoughts as new source contributor like me. This is my first review in my life so sorry if there is any inappropriate. Thanks!

Ok I learn why we say "assign" first... 😓

@knative-prow-robot knative-prow-robot removed the lgtm Indicates that a PR is ready to be merged. label May 20, 2020
@aliok
Copy link
Member Author

aliok commented May 20, 2020

@n3wscott, this needs lgtm again

evankanderson
evankanderson reviewed May 22, 2020
View reviewed changes
Copy link
Member

@evankanderson evankanderson left a comment

Choose a reason for hiding this comment

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

A few formatting bits on the go code (which I'm only flagging because it's probably annoying for copy-paste), but otherwise great.

/approve

aliok and others added 3 commits May 27, 2020 11:16
@aliok @evankanderson
Update docs/eventing/samples/writing-receive-adapter-source/03-contro…
b75ace1 
…ller.md

Co-authored-by: Evan Anderson <evan.k.anderson@gmail.com>
@aliok @evankanderson
Update docs/eventing/samples/writing-receive-adapter-source/03-contro…
b888c35 
…ller.md

Co-authored-by: Evan Anderson <evan.k.anderson@gmail.com>
@aliok
Remove an empty line
76851e2
@aliok
Copy link
Member Author

aliok commented May 27, 2020

@evankanderson I committed the suggestions you made. Thanks.

mind reviewing again?

@aliok
Copy link
Member Author

aliok commented May 27, 2020

/test pull-knative-docs-markdown-link-check

1 similar comment
@aliok
Copy link
Member Author

aliok commented May 27, 2020

/test pull-knative-docs-markdown-link-check

@lberk
Copy link
Member

lberk commented May 27, 2020

Thanks @aliok
/lgtm
/approve

@knative-prow-robot knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label May 27, 2020
@knative-prow-robot
Copy link
Contributor

knative-prow-robot commented May 27, 2020

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aliok, evankanderson, lberk, n3wscott

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

@knative-prow-robot knative-prow-robot merged commit fbf41f0 into knative:master May 27, 2020
rhuss pushed a commit to rhuss/docs that referenced this pull request Jun 2, 2020
@aliok @evankanderson @rhuss
Update sample source guide (knative#2475)
3249d74 
* Make sample source guide up-to-date

* Docs for adding event source to eventing-contrib

* Get rid of some whitespace

* Fix broken markdown

* Update docs/eventing/samples/writing-receive-adapter-source/03-controller.md

Co-authored-by: Evan Anderson <evan.k.anderson@gmail.com>

* Update docs/eventing/samples/writing-receive-adapter-source/03-controller.md

Co-authored-by: Evan Anderson <evan.k.anderson@gmail.com>

* Remove an empty line

Co-authored-by: Evan Anderson <evan.k.anderson@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@evankanderson evankanderson evankanderson left review comments

@nachocano nachocano Awaiting requested review from nachocano

+3 more reviewers

@mattmoor mattmoor mattmoor left review comments

@tom24d tom24d tom24d left review comments

@n3wscott n3wscott n3wscott left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@lberk lberk

@n3wscott n3wscott

Labels

cla: yes Indicates the PR's author has signed the CLA. lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@aliok @tom24d @lberk @knative-prow-robot @mattmoor @evankanderson @n3wscott @googlebot