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

Move ConvertToPush method to pubsub.go #512

Merged
merged 4 commits into from
Jan 24, 2020

Conversation

nlopezgi
Copy link
Contributor

Fixes #497

Proposed Changes

Details:

  • Most code migrated from push.go to pubsub.go
  • if Push mode, converter sets content type to "application/json" in order for convertToPush to decode message.
  • All attrs that are part of the message (i.e., no valid extension filtering) are passed to convertToPush
  • migrated tests from push_test.go to pubsub.go +
  • added test that validates convert calls convertToPush appropriately. Note that message Data has to be passed with quotations for decoding to properly work.

@knative-metrics-robot
Copy link

The following is the coverage report on the affected files.
Say /test pull-google-knative-gcp-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
pkg/pubsub/adapter/adapter.go 40.0% 39.7% -0.3
pkg/pubsub/adapter/converters/pubsub.go 88.9% 89.7% 0.8

@knative-prow-robot
Copy link
Contributor

knative-prow-robot commented Jan 24, 2020

@nlopezgi: The following test failed, say /retest to rerun all failed tests:

Test name Commit Details Rerun command
pull-google-knative-gcp-go-coverage abf2bb3 link /test pull-google-knative-gcp-go-coverage

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

}
return &event, nil
}

// pushMessage represents the format Pub/Sub uses to push events.
type pushMessage struct {
Copy link
Member

Choose a reason for hiding this comment

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

thanks for making it private!

@nachocano
Copy link
Member

/lgtm
/approve

Thanks @nlopezgi !!!

@knative-prow-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: nachocano, nlopezgi

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 2b3a523 into google:master Jan 24, 2020
@nachocano
Copy link
Member

@nlopezgi can you cherry-pick it to release-0.12 branch... We will need this there, together with a follow up that I'll start now

nlopezgi pushed a commit to nlopezgi/knative-gcp that referenced this pull request Jan 24, 2020
* move convert to push to pubsub.go

* adding new test

* collapse convertToPush in convertPubSub

* increase testing coverage
@nlopezgi
Copy link
Contributor Author

release-0.12 branch

Created #520 to cherry pick change into release-0.12

knative-prow-robot pushed a commit that referenced this pull request Jan 24, 2020
* move convert to push to pubsub.go

* adding new test

* collapse convertToPush in convertPubSub

* increase testing coverage
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CloudPubSubSource should use PushCompatible mode
4 participants