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

Port Ceph source to adapter/v2. #1255

Merged
merged 4 commits into from
May 27, 2020
Merged

Port Ceph source to adapter/v2. #1255

merged 4 commits into from
May 27, 2020

Conversation

tom24d
Copy link
Contributor

@tom24d tom24d commented May 21, 2020

Fixes #1216

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

Hi @tom24d. Thanks for your PR.

I'm waiting for a knative member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

@knative-prow-robot knative-prow-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label May 21, 2020
mattmoor
mattmoor reviewed May 21, 2020
View reviewed changes
Copy link
Member

@mattmoor mattmoor left a comment

Choose a reason for hiding this comment

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

Produced via:
gofmt -s -w $(find -path './vendor' -prune -o -path './third_party' -prune -o -type f -name '*.go' -print)

@tom24d
Copy link
Contributor Author

tom24d commented May 21, 2020

/hold until exciting 0.15 is cut out!

@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 May 21, 2020
@matzew
Copy link
Member

matzew commented May 21, 2020

/ok-to-test

@knative-prow-robot knative-prow-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels May 21, 2020
@slinkydeveloper
Copy link
Contributor

/assign

mattmoor
mattmoor reviewed May 21, 2020
View reviewed changes
Copy link
Member

@mattmoor mattmoor left a comment

Choose a reason for hiding this comment

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

Produced via:
gofmt -s -w $(find -path './vendor' -prune -o -path './third_party' -prune -o -type f -name '*.go' -print)

ceph/pkg/adapter/adapter_test.go Show resolved Hide resolved
ceph/pkg/adapter/adapter_test.go Outdated Show resolved Hide resolved
slinkydeveloper
slinkydeveloper suggested changes May 21, 2020
View reviewed changes
Copy link
Contributor

@slinkydeveloper slinkydeveloper left a comment

Choose a reason for hiding this comment

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

First pass done, did you tried it locally?

ceph/cmd/receive_adapter/main.go Outdated Show resolved Hide resolved
ceph/pkg/adapter/adapter.go Outdated Show resolved Hide resolved
ceph/pkg/adapter/adapter_test.go Outdated Show resolved Hide resolved
ceph/pkg/adapter/adapter.go Outdated Show resolved Hide resolved
ceph/pkg/adapter/adapter.go Outdated Show resolved Hide resolved
@tom24d
Copy link
Contributor Author

tom24d commented May 22, 2020

Thanks for the review, @slinkydeveloper !

I am just figuring out now that we cannot use ContainerSource/v1alpha1 anymore as a source base because v2 needs the injection of K_SINK, as the same time the ContainerSource/v1alpha2 is not an option because there would be no way to expose itself. Seems like we have to re-write ceph-container-source.yaml with ksvc or deployment+svc. Does it sounds like correct? and to do so in this PR?

One more thing, Does Ceph adapter needs to be accessible from outside of the cluster? I am not really expert for this..

Thanks,

@matzew
Copy link
Member

matzew commented May 25, 2020

/assign @yuvalif

@knative-prow-robot
Copy link
Contributor

@matzew: GitHub didn't allow me to assign the following users: yuvalif.

Note that only knative members, repo collaborators and people who have commented on this issue/PR can be assigned. Additionally, issues/PRs can only have 10 assignees at the same time.
For more information please see the contributor guide

In response to this:

/assign @yuvalif

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.

@matzew
Copy link
Member

matzew commented May 25, 2020

/lgtm
/approve

@knative-prow-robot knative-prow-robot added lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels May 25, 2020
@tom24d tom24d mentioned this pull request May 26, 2020
Closed
slinkydeveloper
slinkydeveloper reviewed May 26, 2020
View reviewed changes
ceph/cmd/receive_adapter/main.go
// if successful, this should be a blocking call
// ends only when the HTTP server exist
adapter.Start(*sink, *listenPort)
adapter.Main("cephsource", cephadapter.NewEnvConfig, cephadapter.NewAdapter)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think these changes requires changing the examples in ceph/samples to use the SinkBinding mechanism

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! You can see that in the refereed PR.

@tom24d tom24d mentioned this pull request May 27, 2020
Merged
@tom24d
Copy link
Contributor Author

tom24d commented May 27, 2020

/retest
/unhold

#1277 follows.

@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 May 27, 2020
@knative-metrics-robot
Copy link

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

File Old Coverage New Coverage Delta
ceph/pkg/adapter/adapter.go 68.6% 66.7% -2.0

slinkydeveloper
slinkydeveloper approved these changes May 27, 2020
View reviewed changes
Copy link
Contributor

@slinkydeveloper slinkydeveloper left a comment •
edited
Loading

Choose a reason for hiding this comment

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

/lgtm

@knative-prow-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: matzew, slinkydeveloper, tom24d

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 [matzew,slinkydeveloper]

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

@tom24d
Copy link
Contributor Author

tom24d commented May 27, 2020

/test pull-knative-eventing-contrib-unit-tests

@slinkydeveloper
Copy link
Contributor

@tom24d please rebase, that issue is fixed on master

@knative-prow-robot knative-prow-robot merged commit e364e45 into knative:master May 27, 2020
@tom24d
Copy link
Contributor Author

tom24d commented May 27, 2020

@slinkydeveloper Ah, merged. Was it all right?

@slinkydeveloper
Copy link
Contributor

No problem, thanks for the contribution!

@tom24d tom24d deleted the ceph_v2_1216 branch May 27, 2020 13:52
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@mattmoor mattmoor mattmoor left review comments

@slinkydeveloper slinkydeveloper slinkydeveloper approved these changes

@n3wscott n3wscott Awaiting requested review from n3wscott

@vaikas vaikas Awaiting requested review from vaikas

Assignees

@matzew matzew

@slinkydeveloper slinkydeveloper

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cla: yes Indicates the PR's author has signed the CLA. lgtm Indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. 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.

Port Ceph source to adapter/v2
7 participants
@tom24d @knative-prow-robot @matzew @slinkydeveloper @knative-metrics-robot @mattmoor @googlebot