ci: Add pull request template and run commitlint on PR title only

[spetrosi]

We now ensure the conventional commits format only on PR titles and not on
commits to let developers keep commit messages targeted for other developers
i.e. describe actual changes to code that users should not care about.
And PR titles, on the contrary, must be aimed at end users.

For more info, see
https://linux-system-roles.github.io/contribute.html#write-a-good-pr-title-and-description

Signed-off-by: Sergei Petrosian spetrosi@redhat.com

[Jakuje]

This template matches only for the enhancement PRs. There are bugfixes and other options. Not even your PR follows this template so I find it kind of artificial.

In the templates, I would propose to have some generic checklists for contributor/reviewer se have for example in libssh (example: https://gitlab.com/libssh/libssh-mirror/-/merge_requests/340). We probably do not need all the steps, but I find some of them usefule, at least for example the following

• Documentation is updated
• Code is modified for a feature
• Tests are implemented/updated

[spetrosi]

This template matches only for the enhancement PRs. There are bugfixes and other options. Not even your PR follows this template so I find it kind of artificial.

It is possible to add different templates, e.g. have different fields requested for fix and for feat, but that's pretty ugly, described in https://stackoverflow.com/questions/73771068/multiple-templates-for-pull-requests-on-github
So users must do couple clicks to navigate to their template, it's not possible to set the template based on the PR title prefix. So it is easier to just have one template and then rely on contributors' common sense to decide how to fill in the template.

In the templates, I would propose to have some generic checklists for contributor/reviewer se have for example in libssh (example: https://gitlab.com/libssh/libssh-mirror/-/merge_requests/340). We probably do not need all the steps, but I find some of them usefule, at least for example the following

* [ ]  Documentation is updated

* [ ]  Code is modified for a feature

* [ ]  Tests are implemented/updated

I think that would be an overkill to impose so much control, not all commits require this. For now, the aim is to make pr description to have enough information to eventually put it into doc text field in BZ or Jira and have it reviewed by writers without the need to ping developers who worked on the subject several months ago.
And of course get PR title and description to changelog.

[richm]

This template matches only for the enhancement PRs. There are bugfixes and other options. Not even your PR follows this template so I find it kind of artificial.

It is possible to add different templates, e.g. have different fields requested for fix and for feat, but that's pretty ugly, described in https://stackoverflow.com/questions/73771068/multiple-templates-for-pull-requests-on-github So users must do couple clicks to navigate to their template, it's not possible to set the template based on the PR title prefix. So it is easier to just have one template and then rely on contributors' common sense to decide how to fill in the template.

It would help if the template stated that - maybe some text like

# This template is for enhancements because there isn't an easy way to have different templates automatically
# selected for different PR types.  Therefore, use your best judgement.  Note that for features and bug fixes, it is
# very useful for end users to have a detailed description.

Then the tooling could strip out the lines matching ^#

In the templates, I would propose to have some generic checklists for contributor/reviewer se have for example in libssh (example: https://gitlab.com/libssh/libssh-mirror/-/merge_requests/340). We probably do not need all the steps, but I find some of them usefule, at least for example the following

* [ ]  Documentation is updated

* [ ]  Code is modified for a feature

* [ ]  Tests are implemented/updated

I think that would be an overkill to impose so much control, not all commits require this. For now, the aim is to make pr description to have enough information to eventually put it into doc text field in BZ or Jira and have it reviewed by writers without the need to ping developers who worked on the subject several months ago. And of course get PR title and description to changelog.

I think having a checklist is a separate issue, and a good idea - I just recently had to go back and request a kdump developer to add tests to a PR because I missed them - maybe have a 4th checklist item like

* [] None of the above - not applicable for this PR

or something like that - for tests only PRs, ci, docs, etc.

[Jakuje]

The template supports comments using HTML comments like <!-- commment --> so the instructions can be written like this.

We have similar checklist in opensc with comments: https://github.com/OpenSC/OpenSC/blob/master/.github/PULL_REQUEST_TEMPLATE.md?plain=1 with instructions to remove non-applicable items so that would be also an option: <!-- Remove items that do not apply. For completed items, change [ ] to [x]. -->

[richm]

The html comments are preserved in the pr body e.g.

        "body": "fixes https://github.com/OpenSC/OpenSC/issues/2444\r\n\r\n<!--\r\nThank you for your pull request.\r\n\r\nIf this fixes a GitHub issue, make sure to have a line saying 'Fixes #XXXX'\r\n(without quotes) in the commit message.\r\n\r\nMention which card(s) are used during testing. To get the name of your card,\r\nrun this command: `opensc-tool -n`\r\n-->\r\n\r\n##### Checklist\r\n<!-- Remove items that do not apply. For completed items, change [ ] to [x]. -->\r\n- [ ] Documentation is added or updated\r\n- [ ] New files have a LGPL 2.1 license statement\r\n- [ ] PKCS#11 module is tested\r\n- [ ] Windows minidriver is tested\r\n- [ ] macOS tokend is tested\r\n",
        "mergedAt": "2023-02-27T10:39:08Z",
        "number": 2501,
        "title": "enable use_file_cache by default"

the html style comments would make automated tooling difficult - I would prefer to use lines beginning with ^# as comments to make it easy for the automated tooling to strip out

[richm]

@Jakuje another option for the PR template - have a single template with 3 different sections - feature, fix, and other:

# * Describe your change as a New Feature, Bug Fix, or Other 
# * use one of the sections below as a template
# * delete the sections you do not use. 
# * if you are not sure which one to use, ask a role maintainer.
# * if this change addresses github issues or issues in another tracking system, include the
#    links to those issues in the last section

# Section - New Features - use this section to describe a new feature
Enhancement:

Reason:

Result:

# Section - Bug Fixes - use this section to describe a bug fix
Cause: 

Consequence: 

Fix: 

Result: 

# Section - Other - describe a change that isn't a feature or a bug fix
Description: 

# Section - Links to issues
Issue Tracker Tickets (Jira or BZ if any):

too verbose?

[Jakuje]

@richm I see your proposal as an improvement. I think there certainly need to be some instructions otherwise people will get scared or discouraged when they will not know what to do with the 4 words template.

I am worried that the # if they will not be removed from the issue description, they will be rendered as markdown headers and will result in very huge and ugly text. Yeah, this can be fixed afterward, but still ... This is why I would prefer to use the HTML comments (they can be also removed by automation, don't they?).

I am also worried that the doc text templates as defined in bugzilla need some practice to fill and even more practice to make sense and be useful. So for new contributors, this might be a burden. But having the "other" option available (and instructions) should help. Its long, but I think understandable.

[spetrosi]

I think that having Enhancement, Reason, and Result is enough for a reasonable changelog entry. Comparing to Cause, Consequence, Fix, and Result for bug fix, cause+consequence= result, cause=reason, result=result. Having more options, like Rich suggested, puts the burden of removing all unrelated fields. My intention was to make it as simple as possible so that devs just fill it in.

[richm]

This is why I would prefer to use the HTML comments (they can be also removed by automation, don't they?).

I don't know - do we need an HTML parser in order to do this? I mean, I'm sure you can come up with some really complex sed regular expressions to handle single line <!-- ..... --> and multiline <!-- ....\n....\n...\n... -->, but removing # with /^#/d is quite simple

[richm]

I think that having Enhancement, Reason, and Result is enough for a reasonable changelog entry. Comparing to Cause, Consequence, Fix, and Result for bug fix, cause+consequence= result, cause=reason, result=result. Having more options, like Rich suggested, puts the burden of removing all unrelated fields. My intention was to make it as simple as possible so that devs just fill it in.

I second that. Let's see how it works in practice - if it is too hard to understand, or too onerous, then we can revisit.

[Jakuje]

removing # with /^#/d is quite simple

but in that case you would remove also the markdown headers, isn't it?

I second that. Let's see how it works in practice - if it is too hard to understand, or too onerous, then we can revisit.

Ok. Fine for me. Lets move on with the current state, but lets keep options for the future.

[richm]

removing # with /^#/d is quite simple

but in that case you would remove also the markdown headers, isn't it?

Hmmm - I hadn't considered users putting markdown headers in the PR body - is that a common thing to do? I haven't really seen it.

For changelog automation, we only care about the PR title/header, not the body.

I second that. Let's see how it works in practice - if it is too hard to understand, or too onerous, then we can revisit.

Ok. Fine for me. Lets move on with the current state, but lets keep options for the future.

+1