Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

/area /kind commands are still effective in html comments <!-- and --> #8937

Closed
ahmetb opened this issue Aug 3, 2018 · 3 comments
Closed
Labels
area/prow Issues or PRs related to prow

Comments

@ahmetb
Copy link
Member

ahmetb commented Aug 3, 2018

At knative/serving#1797
the issue template shows users all /area and /kind commands, in a <!-- --> block

If users don't delete that, all labels are accidentally added.

Can prow detect and ignore whatever is in comment blocks?

@BenTheElder
Copy link
Member

k/k's template puts these like:

uncomment one of the following:
> /foo bar
> /foo baz

Right now the commands are simple regexes against the comments like (?mi)^/lgtm(?: no-issue)?\s*$. Even just indenting the unwanted commands should work since we match on ^/command typically.

Alternatively we can add prepreocessing to prow to match and eliminate HTML comment blocks from comments before matching comments. I'd favor just fixing the template to not match though, over adding an HTML parser to prow's comment handling...

cc @cjwagner
/area prow
/kind enhancement

@k8s-ci-robot k8s-ci-robot added the area/prow Issues or PRs related to prow label Aug 3, 2018
@grantr
Copy link
Contributor

grantr commented Aug 3, 2018

The purpose of the comment in the template is to hide /area commands and get a cleaner issue body. There's no need to make Prow ignore comments - currently we want Prow to not ignore comments!

The comments seem like they might cause more confusion (for issue creators and subsequent viewers) than the lack of command clutter is worth. @mattmoor WDYT about removing them from the template?

@ahmetb
Copy link
Member Author

ahmetb commented Aug 3, 2018

I guess that makes sense. Feel free to close this.

@cjwagner cjwagner closed this as completed Aug 3, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/prow Issues or PRs related to prow
Projects
None yet
Development

No branches or pull requests

5 participants