-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
feat: add run_events_pubsub #1809
Conversation
Signed-off-by: Grant Timmerman <timmerman+devrel@google.com>
I don't understand why the tests have failed for the md preview renderer. It seems unrelated to this PR.
That is frustrating. |
Looks like we should get those kokoro files merged ASAP @kelsk @grayside |
Grant, the failure for |
run/README.md
Outdated
@@ -16,6 +16,7 @@ | |||
|[Cloud SQL (MySQL)][mysql] | Use MySQL with Cloud Run | - | | |||
|[Cloud SQL (Postgres)][postgres] | Use Postgres with Cloud Run | - | | |||
|[Hello Broken][hello_broken] | Something is wrong, how do you fix it? | [<img src="https://storage.googleapis.com/cloudrun/button.svg" alt="Run on Google Cloud" height="30"/>][run_button_hello_broken] | | |||
|[Events – Pub/Sub][events_pubsub] | Use Pub/Sub with Cloud Run | - | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should have this description parallel the Pub/Sub description more, and be moved right under it. Might want to flag it as alpha if we're going to "advertise" it here.
|[Events – Pub/Sub][events_pubsub] | Use Pub/Sub with Cloud Run | - | | |
|[Events – Pub/Sub][events_pubsub] | Event-driven service with Events for Cloud Run for Pub/Sub | - | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree we can move the Pub/Sub trigger next to the other Pub/Sub sample.
I disagree that we should change the title in this way.
Cloud Run is not_for_ Pub/Sub.
Pub/Sub is a trigger/event type for Cloud Run.
Once public, we should remove the non-native Pub/Sub sample.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The core feedback here is that the two samples should be listed next to each other, and with similar enough structure in their descriptions to make differentiation easy.
Whether one of them is removed at GA is a good question.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the reviews!
Addressed a few issues.
Need to sync in the design doc a bit more before asking for another review.
run/README.md
Outdated
@@ -16,6 +16,7 @@ | |||
|[Cloud SQL (MySQL)][mysql] | Use MySQL with Cloud Run | - | | |||
|[Cloud SQL (Postgres)][postgres] | Use Postgres with Cloud Run | - | | |||
|[Hello Broken][hello_broken] | Something is wrong, how do you fix it? | [<img src="https://storage.googleapis.com/cloudrun/button.svg" alt="Run on Google Cloud" height="30"/>][run_button_hello_broken] | | |||
|[Events – Pub/Sub][events_pubsub] | Use Pub/Sub with Cloud Run | - | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree we can move the Pub/Sub trigger next to the other Pub/Sub sample.
I disagree that we should change the title in this way.
Cloud Run is not_for_ Pub/Sub.
Pub/Sub is a trigger/event type for Cloud Run.
Once public, we should remove the non-native Pub/Sub sample.
Signed-off-by: Grant Timmerman <timmerman+devrel@google.com>
OK. Please advise on how to proceed. It's really hard to understand the PR test status if |
…orm/nodejs-docs-samples into grant_run_events_pubsub
Signed-off-by: Grant Timmerman <timmerman+devrel@google.com>
I've created GitHub issues for unrelated failing tests. |
Signed-off-by: Grant Timmerman <timmerman+devrel@google.com>
Signed-off-by: Grant Timmerman <timmerman+devrel@google.com>
hello-broken failing is news to me, thanks for filing the issue. The other two are part of #1752, it seems like Kokoro was configured to expect that PR to already be merged. Note that while it's a failure, it's not a "required" check, so won't block merging. |
run/README.md
Outdated
@@ -16,6 +16,7 @@ | |||
|[Cloud SQL (MySQL)][mysql] | Use MySQL with Cloud Run | - | | |||
|[Cloud SQL (Postgres)][postgres] | Use Postgres with Cloud Run | - | | |||
|[Hello Broken][hello_broken] | Something is wrong, how do you fix it? | [<img src="https://storage.googleapis.com/cloudrun/button.svg" alt="Run on Google Cloud" height="30"/>][run_button_hello_broken] | | |||
|[Events – Pub/Sub][events_pubsub] | Use Pub/Sub with Cloud Run | - | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The core feedback here is that the two samples should be listed next to each other, and with similar enough structure in their descriptions to make differentiation easy.
Whether one of them is removed at GA is a good question.
Signed-off-by: Grant Timmerman <timmerman+devrel@google.com>
@grayside PTAL |
Adds a minimal Events for Cloud Run for Pub/Sub triggers.
Similar to the existing Pub/Sub sample, but uses Events for Cloud Run directly. Worth having a separate sample (at least until GA).
Future samples will be prefixed with:
run_events_{trigger type}
run_events_pubsub
run_events_audit_logs_gcs
run_events_firestore
...etc.