Skip to content
This repository has been archived by the owner on Jun 4, 2021. It is now read-only.

GCP PubSub Source -- Pull #39

Merged
merged 45 commits into from
Nov 2, 2018
Merged

GCP PubSub Source -- Pull #39

merged 45 commits into from
Nov 2, 2018

Conversation

adamharwayne
Copy link
Contributor

Working towards knative/eventing#450 and knative/eventing#451.

Proposed Changes

  • Create a Source for GCP PubSub topics.
    • This source uses the pull model for subscriptions, so needs to continuously run.

Release Note

NONE

@knative-prow-robot knative-prow-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 26, 2018
@knative-prow-robot knative-prow-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Oct 26, 2018
adamharwayne
adamharwayne commented Oct 26, 2018
View reviewed changes
config/manager/manager.yaml Outdated Show resolved Hide resolved
config/pubsub.yaml Outdated Show resolved Hide resolved
@grantr grantr mentioned this pull request Oct 26, 2018
Merged
@knative-prow-robot knative-prow-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm Indicates that a PR is ready to be merged. labels Nov 1, 2018
@knative-prow-robot knative-prow-robot removed the lgtm Indicates that a PR is ready to be merged. label Nov 1, 2018
@vaikas
Copy link
Contributor

vaikas commented Nov 1, 2018

/lgtm
/hold

@knative-prow-robot knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Nov 1, 2018
@adamharwayne
Copy link
Contributor Author

I think I have addressed all the comments. @n3wscott and @grantr is there anything else to address before merging?

grantr
grantr approved these changes Nov 1, 2018
View reviewed changes
Copy link
Contributor

@grantr grantr left a comment

Choose a reason for hiding this comment

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

/lgtm

Great work here as usual @adamharwayne (omg tests!). Good experience and a good example for future source developers. Thanks!

pkg/apis/sources/v1alpha1/gcp_pubsub_validation.go Outdated Show resolved Hide resolved
@knative-prow-robot knative-prow-robot removed the lgtm Indicates that a PR is ready to be merged. label Nov 1, 2018
@adamharwayne adamharwayne mentioned this pull request Nov 1, 2018
Closed
@vaikas
Copy link
Contributor

vaikas commented Nov 1, 2018

/lgtm
/approve

@knative-prow-robot knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Nov 1, 2018
@knative-prow-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: adamharwayne, grantr, vaikas-google

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:
  • OWNERS [grantr,vaikas-google]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@grantr
Copy link
Contributor

grantr commented Nov 1, 2018

@n3wscott can we remove the hold on this?

@Harwayne
Each source can now specify its own receive adapter's service account…
384cb25 
…, rather than using one for all receive adapters.
@knative-prow-robot knative-prow-robot removed the lgtm Indicates that a PR is ready to be merged. label Nov 1, 2018
@knative-metrics-robot
Copy link

The following is the coverage report on pkg/.
Say /test pull-knative-eventing-sources-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
cmd/gcppubsub_receive_adapter/main.go Do not exist 0.0%
cmd/message_dumper/dumper.go Do not exist 0.0%
pkg/apis/sources/v1alpha1/gcp_pubsub_types.go Do not exist 100.0%
pkg/apis/sources/v1alpha1/gcp_pubsub_validation.go Do not exist 87.5%
pkg/controller/add_gcppubsubsources.go Do not exist 100.0%

@vaikas
Copy link
Contributor

vaikas commented Nov 2, 2018

/lgtm
/approve

@knative-prow-robot knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Nov 2, 2018
@n3wscott
Copy link
Contributor

n3wscott commented Nov 2, 2018

/lgtm

@n3wscott
Copy link
Contributor

n3wscott commented Nov 2, 2018

/hold cancel

@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 Nov 2, 2018
@knative-prow-robot knative-prow-robot merged commit 3d03e64 into knative:master Nov 2, 2018
@n3wscott n3wscott mentioned this pull request Mar 18, 2019
Merged
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@grantr grantr grantr approved these changes

@vaikas vaikas vaikas left review comments

@n3wscott n3wscott n3wscott left review comments

@evankanderson evankanderson Awaiting requested review from evankanderson

Assignees

@grantr grantr

@vaikas vaikas

@n3wscott n3wscott

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@adamharwayne @vaikas @knative-prow-robot @grantr @knative-metrics-robot @n3wscott @Harwayne