Skip to content

Conversation

@n3wscott
Copy link
Contributor

Dependent on knative/eventing#2427

Document the renamed instructions for PingSource. Formally called CronJobSource.

@googlebot googlebot added the cla: yes Indicates the PR's author has signed the CLA. label Jan 23, 2020
@knative-prow-robot knative-prow-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jan 23, 2020
@matzew
Copy link
Member

matzew commented Jan 23, 2020

/lgtm

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

Hey @n3wscott ! Should we also delete the references to CronJobSource in the docs when we merge this in?

@n3wscott
Copy link
Contributor Author

Hey @n3wscott ! Should we also delete the references to CronJobSource in the docs when we merge this in?

Not yet, wait until post 0.14 release.

Copy link
Contributor

@samodell samodell left a comment

Choose a reason for hiding this comment

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

Oof, looks like I wrote these comments and never sent them out. Sorry for the delay on this!

@knative-prow-robot knative-prow-robot removed the lgtm Indicates that a PR is ready to be merged. label Feb 26, 2020
Copy link
Contributor

@carieshmarie carieshmarie left a comment

Choose a reason for hiding this comment

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

Thanks for your PR! Just a few minor comments/requested changes.


## Cleanup

You can delete the PingSource by entering the following command:
Copy link
Contributor

Choose a reason for hiding this comment

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

What exactly are you deleting here? Is it the event source? Or the YAML file? Can we add a noun to the sentence? i.e. "You can delete the Pingsource [NOUN] by entering the following command..."

Copy link
Contributor

Choose a reason for hiding this comment

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

My guess is the event source is created the same way as a service by applying a yaml file, so deleting the yaml file is deleting the source effectively?

Copy link
Contributor

@abrennan89 abrennan89 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 few additional comments

---

PingSource example shows how to configure PingSource as event source for
functions.
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be services or Knative services rather than "functions"?


## Create a PingSource

For each set of ping events you want to request, you need to create an Event
Copy link
Contributor

Choose a reason for hiding this comment

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

"For each set of ping events you want to request" - would someone set up a bunch of different PingSource event sources though for each service, or would they have one PingSource and then use the broker and filters to send the events to different services? I don't think it's clear how this is used.


## Cleanup

You can delete the PingSource by entering the following command:
Copy link
Contributor

Choose a reason for hiding this comment

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

My guess is the event source is created the same way as a service by applying a yaml file, so deleting the yaml file is deleting the source effectively?

@@ -0,0 +1,11 @@
# This is a very simple Knative Service that writes the input request to its log.
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe mention that it's an event input request or something?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it is used all over, maybe we can migrate to a central doc to talk about this?

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe time to resurrect #170 ?

@abrennan89 abrennan89 removed the request for review from RichieEscarez February 27, 2020 14:00
@abrennan89
Copy link
Contributor

Hey @n3wscott ! Should we also delete the references to CronJobSource in the docs when we merge this in?

Not yet, wait until post 0.14 release.

@mattmoor should we add an issue to update this to the 0.14 release milestone?

@googlebot
Copy link

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

@googlebot googlebot added cla: no Indicates the PR's author has not signed the CLA. and removed cla: yes Indicates the PR's author has signed the CLA. labels Feb 27, 2020
commit 9a04cb6
Author: Scott Nichols <snichols@vmware.com>
Date:   Thu Feb 27 07:32:03 2020 -0800

    Document ping source.
@n3wscott
Copy link
Contributor Author

@googlebot I fixed it.

@googlebot
Copy link

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

@googlebot googlebot added cla: yes Indicates the PR's author has signed the CLA. and removed cla: no Indicates the PR's author has not signed the CLA. labels Feb 27, 2020
@mattmoor
Copy link
Member

/approve

@mattmoor
Copy link
Member

☝️ I'm triggering stagedoc-bot for scott, so also adding:

/hold

@knative-prow-robot knative-prow-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 27, 2020
@knative-prow-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: abrennan89, mattmoor, 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

@matzew
Copy link
Member

matzew commented Feb 27, 2020

/lgtm

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

@n3wscott is this ready?

@matzew
Copy link
Member

matzew commented Mar 5, 2020

@n3wscott I've updated it, here:

n3wscott#1

feel free to squash it in

@matzew matzew mentioned this pull request Mar 5, 2020
@n3wscott
Copy link
Contributor Author

n3wscott commented Mar 5, 2020

I would like to merge this and then do a pass on the entire codebase updating to v1alpha2 for pingsource

@n3wscott
Copy link
Contributor Author

n3wscott commented Mar 5, 2020

This is ready, has been for a week

@mattmoor
Copy link
Member

mattmoor commented Mar 5, 2020

We should (separately) get the eventing WG leads added to OWNERS for this area of the code. I'm surprised Scotty needs approval here.

@carieshmarie anything else here?

@n3wscott
Copy link
Contributor Author

n3wscott commented Mar 5, 2020

/unhold

@knative-prow-robot knative-prow-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 5, 2020
@matzew
Copy link
Member

matzew commented Mar 9, 2020

/lgtm

@knative-prow-robot knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 9, 2020
@knative-prow-robot knative-prow-robot merged commit c44c757 into knative:master Mar 9, 2020
vaikas pushed a commit to vaikas/docs that referenced this pull request Mar 13, 2020
* Document ping source.

* Squashed commit of the following:

commit 9a04cb6
Author: Scott Nichols <snichols@vmware.com>
Date:   Thu Feb 27 07:32:03 2020 -0800

    Document ping source.

* typo on source.

* update image

* kail tabs

* by name default
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

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

Development

Successfully merging this pull request may close these issues.

9 participants