*: Add documentation to the checklist

[dveeden]

What problem does this PR solve?

Problem Summary:

• Use checkboxes for the checklist, similar to what's being done for pingcap/docs
• Add "Documentation" to the checklist to avoid changes in syntax, variables and experimental features without proper documentation.

Release note

• No release note

[dveeden]

/cc @morgo @TomShawn

[morgo]

LGTM, minor nits.

[tisonkun]

It seems the changes now checklist contains "Documentation" makes ### Related changes section redundant. What do you think?

Also, the template doesn't say what if the entry is invalid. I think quite a few PRs have nothing to do with user documentation or significant impact on side effects. I'd prefer in form

* Affect user behaviors (yes/no)

... which avoids a list of PRs with uncompleted task list (see this page).

[ti-chi-bot]

@tisonkun: Request changes is only allowed for the reviewers in list.

In response to this:

It seems the changes now checklist contains "Documentation" makes ### Related changes section redundant. What do you think?

Also, the template doesn't say what if the entry is invalid. I think quite a few PRs have nothing to do with user documentation or significant impact on side effects. I'd prefer in form

* Affect user behaviors (yes/no)

... which avoids a list of PRs with uncompleted task list (see this page).

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 ti-community-infra/tichi repository.

[tisonkun]

But yes, it is a good PR to go. The redundant we can resolve, the style we can try to reach a consensus.

[dveeden]

It seems the changes now checklist contains "Documentation" makes ### Related changes section redundant. What do you think?

I don't think they are fully redundant. If someone ticks any of the documentation checkboxes and there is no related docs link a reviewer should start to ask some questions. But it would be good to integrate these more.

Also, the template doesn't say what if the entry is invalid. I think quite a few PRs have nothing to do with user documentation or significant impact on side effects. I'd prefer in form

* Affect user behaviors (yes/no)

... which avoids a list of PRs with uncompleted task list (see this page).

I don't see this as a task list that should eventually have all boxes ticked, but that's how GitHub displays it.

I rather have something were I tick boxes than something were I have to remove lines from the template. Also as a reviewer it would be better to see that someone didn't tick a box then to have to detect that lines were removed.

Making this a yes/no question would be good, but that's not all that different from ticked/unticked or is it?

Some alternatives for the following:

- [ ] Contains variable changes
- [ ] Contains experimental features

Alternative 1:

- [ ] All variable changes are documented
- [ ] All experimental features are labeled as such

Alternative 2:

- [ ] All variable changes are documented
<!-- Add link to variable changes docs here ->
- [ ] All experimental features are labeled as such
<!- Add link to experimental features docs here ->

Alternative 3:

<!-- if you tick any of the boxes you should add a link to the documentation PR ->
- [ ] Contains variable changes
- [ ] Contains experimental features

I think there are two things we want from this:

1. Have the one filing the PR to consider if documentation needs updating
2. Give more information to the reviewer to see if documentation needs updating

[tisonkun]

@dveeden thanks for your reply. I'll check and reply tomorrow.

[tisonkun]

But it would be good to integrate these more.

Yep. That is what I mean. It is better not to have two sections with overlapping info.

Also as a reviewer it would be better to see that someone didn't tick a box then to have to detect that lines were removed.

Not really. It is a "default" topic. If the item removed, it means you should not care about it. A reviewer looking for a certain info said document will found the description missing and ask for it; otherwise the info "this is unrelated to document" is obsolete.

---

But I think we can postpone the discussion. Focusing this pull request, the blockers from my perspective is that we should merge two document related sections. And by the way remove "Need to cherry-pick to the release branch" line since we always and already tracked it by labels.

That is, dealing with "Related changes" section whose "PR to update pingcap/docs/pingcap/docs-cn" item affected by this pull request.

Other parts looks good to me. We don't expand a lot the scope of this pull request.

[ti-chi-bot]

@tisonkun: Request changes is only allowed for the reviewers in list.

In response to this:

But it would be good to integrate these more.

Yep. That is what I mean. It is better not to have two sections with overlapping info.

Also as a reviewer it would be better to see that someone didn't tick a box then to have to detect that lines were removed.

Not really. It is a "default" topic. If the item removed, it means you should not care about it. A reviewer looking for a certain info said document will found the description missing and ask for it; otherwise the info "this is unrelated to document" is obsolete.

---

But I think we can postpone the discussion later. Focusing this pull request, the blockers from my perspective is that we should merge two document related sections. And by the way remove "Need to cherry-pick to the release branch" line since we always and already tracked it by labels.

That is, dealing with "Related changes" whose "PR to update pingcap/docs/pingcap/docs-cn" item affected by this pull request.

Other parts looks good to me. We don't expand a lot the scope of this pull request.

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 ti-community-infra/tichi repository.

[tisonkun]

I've sent a pull request to this pr dveeden#1. Generally we just remove that section and it is ok because no one fill those items.

To tracking the sibling pr on docs repo, I'd prefer using a tracking issue or link from the issue spawning the pr instead of link between prs. Anyway, no one fill that item.

[tisonkun]

LGTM. Thanks for your contribution!

[ti-chi-bot]

@tisonkun: Thanks for your review. The bot only counts approvals from reviewers and higher roles in list, but you're still welcome to leave your comments.

In response to this:

LGTM. Thanks for your contribution!

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 ti-community-infra/tichi repository.

[QueenyJin]

LGTM.

[tisonkun]

/run-all-tests

[TomShawn]

LGTM

[tisonkun]

@TomShawn could you please help on trigger merging?

[tisonkun]

/run-check_dev_2

[TomShawn]

/run-all-tests

[TomShawn]

/merge

[ti-chi-bot]

@TomShawn: /merge in this pull request requires 2 approval(s).

In response to this:

/merge

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 ti-community-infra/tichi repository.

[TomShawn]

LGTM

[ti-chi-bot]

[REVIEW NOTIFICATION]

This pull request has been approved by:

• TomShawn
• morgo

To complete the pull request process, please ask the reviewers in the list to review by filling /cc @reviewer in the comment.
After your PR has acquired the required number of LGTMs, you can assign this pull request to the committer in the list by filling /assign @committer in the comment to help you merge this pull request.

The full list of commands accepted by this bot can be found here.

Reviewer can indicate their review by submitting an approval review.
Reviewer can cancel approval by submitting a request changes review.

[TomShawn]

/merge

[ti-chi-bot]

This pull request has been accepted and is ready to merge.

Commit hash: 613f49a

[ti-chi-bot]

@dveeden: Your PR was out of date, I have automatically updated it for you.

At the same time I will also trigger all tests for you:

/run-all-tests

If the CI test fails, you just re-trigger the test that failed and the bot will merge the PR for you after the CI passes.

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 ti-community-infra/tichi repository.