search

knative / client-contrib

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

[kn-admin] support downloading profiling data for knative components #72

Closed xliuxu closed 4 years ago

xliuxu commented 4 years ago

This is a draft PR for @chaozbj and I to co-work to implement a profiling subcommand for kn-admin plugin. Details refer to https://github.com/knative/client-contrib/issues/66

knative-prow-robot commented 4 years ago

Hi @lanceliuu. 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](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

/ok-to-test

xliuxu commented 4 years ago

@maximilien I will fill up tests and then request for review. Thanks for your comments!

xliuxu commented 4 years ago

/retest

knative-prow-robot commented 4 years ago

@lanceliuu: The /test command needs one or more targets. The following commands are available to trigger jobs:

Use /test all to run all jobs.

In response to [this](https://github.com/knative/client-contrib/pull/72#issuecomment-663986498): >/test > 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.
maximilien commented 4 years ago

Hi, @lanceliuu, is this ready for new review? Does not seem like PROW ran any tests? Please tag me when ready for another review. Thanks.

xliuxu commented 4 years ago

/retest

xliuxu commented 4 years ago

@maximilien @zhanggbj This PR is ready for review. Thanks! e2e tests for this command is currently missing since it depends on https://github.com/knative/client-contrib/pull/63

zhanggbj commented 4 years ago

@lanceliuu @chaozbj Thanks for the PR. Looks great! I just left some comments as above.

maximilien commented 4 years ago

Please address @zhanggbj comments and I can take a final pass after. Thx.

maximilien commented 4 years ago

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

Might want to see why ^^^ is / was failing as well.

chaozbj commented 4 years ago

Please hold for a while, I will improve wording for profiling.go file and submit the changes tonight. Thanks!

chaozbj commented 4 years ago

@maximilien and @zhanggbj I addressed Grace's comments for profiling.go and README.adoc files:

  1. Changed profile to profiling in command help and output.
  2. Added Serving after Knative word in command help and output to make its scope more clear.
xliuxu commented 4 years ago

@maximilien @zhanggbj I have rebased this PR and add e2e tests. PTAL.

knative-prow-robot commented 4 years ago

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: lanceliuu, zhanggbj

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

@lanceliuu @chaozbj Nice and thanks for the PR! Looks good to me. I just approved it. Wait for @maximilien if any more comment.

zhanggbj commented 4 years ago

Look like no more comments, so I'll merge it now, thanks! /lgtm

zhanggbj commented 4 years ago

Hi @lanceliuu @chaozbj , already LGTM, would you please help to rebase it? Thanks!

xliuxu commented 4 years ago

@zhanggbj Rebase done. Thank you.

zhanggbj commented 4 years ago

/lgtm Great! Thanks!