kubernetes-sigs / prow

Prow is a Kubernetes based CI/CD system developed to serve the Kubernetes community. This repository contains Prow source code and Hugo sources for Prow documentation site.
https://docs.prow.k8s.io
Apache License 2.0
129 stars 99 forks source link

trigger: Improve experience of guided comment #321

Closed wuhuizuo closed 6 days ago

wuhuizuo commented 1 week ago

https://github.com/kubernetes-sigs/prow/blob/b21d1cf485474d6da2623ccc40849e2c8b24b2e5/pkg/pjutil/help.go#L75

Maybe we can make the line copied able in GitHub PR comment.

Many people are used to copying the instructions given in the comments and then creating new trigger comments, but it is easy to copy the leading blank characters, which will cause the trigger plugin to reject such instructions. By introducing the click-copy feature, this situation will be effectively avoided, and it will be more convenient to use.

In terms of the solution, this does not need to rely on any other third-party functions. GitHub markdown itself supports this capability. We only need to fine-tune the markdown format to do it.

The following is a comparison of the effects.

Before:

@wuhuizuo: The specified target(s) for `/test` were not found. The following commands are available to trigger required jobs:

* `/test pull-unit-test`
image

After:

@wuhuizuo: The specified target(s) for `/test` were not found. The following commands are available to trigger required jobs:

*     /test pull-unit-test

image

The different between the two markdown contents:

3c3
< * `/test pull-unit-test`
---
> *     /test pull-unit-test
### Tasks
petr-muller commented 1 week ago

I kinda like this - it looks better especially with short lists. I am a bit concerned about how much space this takes when you have 10+ jobs configured, which is not uncommon.

If we do this then we no longer need to use bullet points though, and then we can easily use the triple ticks instead of the four-blank indent, like this:

```
/test one
```
```
/test two
```

which gets rendered into:

/test one
/test two
petr-muller commented 1 week ago

I can see us using this markup even in other places that suggest commands

wuhuizuo commented 1 week ago

I can see us using this markup even in other places that suggest commands

Yes, it's better to replace the entire thing at once.

petr-muller commented 1 week ago

Yes, it's better to replace the entire thing at once.

That's not what I wanted to suggest. I simply like the proposal. I'm happy to accept multiple smaller PRs, not necessarily one big one.

petr-muller commented 1 week ago

@wuhuizuo do you want to do this PR for trigger?

wuhuizuo commented 1 week ago

@wuhuizuo do you want to do this PR for trigger?

Yes

wuhuizuo commented 1 week ago

/assign

wuhuizuo commented 1 week ago

@petr-muller I have contributed a PR, PTAL: