search

knative / client-contrib

Community contributed `kn` plugins.
Apache License 2.0
10 stars 23 forks source link

[kn-source-kafka] Add kafka source command group #33

Closed daisy-ycguo closed 4 years ago

daisy-ycguo commented 4 years ago

Including:

knative-prow-robot commented 4 years ago

@daisy-ycguo: you cannot LGTM your own PR.

In response to [this](https://github.com/knative/client-contrib/pull/33#issuecomment-634007638): >/lgtm Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.
daisy-ycguo commented 4 years ago

/retest

daisy-ycguo commented 4 years ago

/test pull-knative-client-contrib-integration-tests

rhuss commented 4 years ago

@daisy-ycguo thanks ! Sorry for the delay, your CI fix should be merged now and then let's try if the CI is working again. Before merging this PR I would suggest that we wait to check out whether we can work without vendoring (working with @zhanggbj to try this out). If so, we should already switch to this mode here, as I'm not sure if not already messed up the Git history with all the vendoring classes. We probably would need to clean that up, as a git clone will also fetch the history, even the code that has been deleted. If we switch to non-vendoring mode we would need to find a way how we could cleanup the history.

rhuss commented 4 years ago

/retest

daisy-ycguo commented 4 years ago

Thank you @rhuss . The tests are passed. This PR is ready for review. @maximilien please help to review when you are available.

rhuss commented 4 years ago

Thanks @daisy-ycguo ! we have a public holiday today (and also a free day tomorrow) so that I can look at in on Monday

maximilien commented 4 years ago

@daisy-ycguo you need to also address the lintting bot's comments. It's various minor issues that you should be able to address quickly.

Let me know after and I'll take another pass. Best.

rhuss commented 4 years ago

@daisy-ycguo you need to also address the lintting bot's comments. It's various minor issues that you should be able to address quickly.

Unfortunately the bot also croaks about issues in vendor dirs, which we can't really fix. So please ignore those suggestion which are located in the vendor dir.

daisy-ycguo commented 4 years ago

@daisy-ycguo you need to also address the lintting bot's comments. It's various minor issues that you should be able to address quickly.

Unfortunately the bot also croaks about issues in vendor dirs, which we can't really fix. So please ignore those suggestion which are located in the vendor dir.

Are we able to disable this kind of check? or are we going to disable ? @rhuss

rhuss commented 4 years ago

Are we able to disable this kind of check? or are we going to disable ? @rhuss

We have to check with @mattmoor (i.e. to add support for non top-level vendor dirs as it is the case here) but when are removing vendoring anyway, the bots should be much less noisy.

maximilien commented 4 years ago

Please address @navidshaikh comments. Once done LGTM

maximilien commented 4 years ago

/approve

daisy-ycguo commented 4 years ago

@navidshaikh thank you for the review. I have addressed your comments. To answer your question about README.md out of sync, it is because that README.md is auto generated when building but the auto generated README.md cannot meet with the format review by mattmoore. So I updated README.md manually after building. Based on our discussion in slack, I will stick with the auto generated version of README.md and ignore the format review comments. I update e2e test script a little to apply kafka topic yaml file when setting up kafka cluster also.

maximilien commented 4 years ago

@navidshaikh LGTM so feel free to check @daisy-ycguo’s changes and merge when you feel ready. Best.

navidshaikh commented 4 years ago

/lgtm /approve

lets get this in and work on

in next iterations.

knative-prow-robot commented 4 years ago

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: daisy-ycguo, maximilien, navidshaikh

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](https://github.com/knative/client-contrib/blob/master/OWNERS)~~ [maximilien,navidshaikh] Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment
navidshaikh commented 4 years ago