Discussion: When to add tags to Presto git commits?

[tdcmeehan]

In this project, we will add tags for certain commits (the tag takes the form of a small message in square brackets in the title of the commit). Running git log | grep -oE ' \[.*?\]' | sort | uniq in the commit history, I see the following:

    [Cassandra doc]
    [GH Actions]
    [GeoSpatial]
    [INFO]
    [INNER|LEFT|RIGHT|FULL]
    [Iceberg]
    [NPE in UnaryUnionOp]
    [Native]
    [Presto-on-Spark Native]
    [Presto-on-Spark]
    [Presto-on-spark native]
    [QueryId]
    [TEST]
    [Test Only]
    [Verifier]
    [^;]
    [ci]
    [https://github.com/prestodb/presto/issues/10739]
    [issue-19030]
    [local]
    [maven-release-plugin]
    [naitve]
    [natice pos]
    [natie]
    [native pos]
    [native-pos]
    [native]
    [natvie]
    [pos]
    [prestissimo]
    [presto-native]
    [presto-orc]
    [presto]
    [refactor]
    [regionkey$gid:bigint, groupid:bigint, $hashvalue:bigint, count:bigint]
    [remote]
    [spark]
    [thrift connector]
    [tmp_presto_9463216b05bc4223a243554a6933634b]
    [velox]

I am asking the community the following questions, as I myself am unaware:

• Why do we add such tags?
• Is it possible to standardize on what tags we add, so that they can be documented and validated at commit time? It is presently undocumented when to tag.

If we can't answer both questions, should we stop tagging?

[majetideepak]

I don't know the history, but for me, tags are helpful to filter specific PRs eg: related to [Native] from my GitHub notifications.
We should standardize on what tags to add. For example, the tag [regionkey$gid:bigint, groupid:bigint, $hashvalue:bigint, count:bigint] from your list above is not helpful. We should document and validate them at commit time.

[majetideepak]

Some thoughts on the standardization.
A tag should cover a broad area such as Prestissimo, build, ci, or a connector.
There should be one tag for a given area and in lowercase. From your list, Presto on Spark has many variations.
A new tag can be proposed by opening an issue.
Reviewers must ensure the standardization is followed.
Examples: [native], [native-pos], [connector-name], [build], [ci].
We could combine two tags if needed. example: [native][build]

[majetideepak]

Some of the PRs could be experimental and I see they are tagged [TEST], [WIP], [TEST-ONLY].
Such PRs should be opened as Drafts.

[tdcmeehan]

I have doubts that leaning on reviewers is a good way to go here. Some examples where this ought to happen now but we fall short:

• Release notes -- reviewers consistently fail to validate release notes, even though it's in our PR checklist now
• Commit guidelines besides tags -- again, reviewers consistently fail to validate that commit messages follow our guidelines
• Docs -- reviewers consistently fail to mandate that proper Javadoc or documentation updates are added. We're getting better here, but it's still a big problem for us.

I think it's less error prone to make it more foolproof--that's why, if we really want tags, I think it's best to add some sort of list somewhere that a static analyzer checks against to ensure that the tag is valid.

[steveburnett]

If we create an automated validation process for tags, I suggest that we document the list of valid tags, and make awareness of the tags list a part of the contribution process. Contributors should be able to find what the correct format and content of tags are before they are blocked by an error message. (A similar example is a “Create a password” field that reveals you broke their rules only after you enter and submit a password.) Let’s give people the ability to succeed.

A good place to add a topic on Tags would be as part of the Commit Formatting and Pull Requests topic in Review and Commit guidelines. A Tags topic could contain the following information (draft here summarized from the previous comments):

Format of tag:
* square brackets
* at beginning of title
* lowercase
* can have more than one tag in a title 
(Note: in the same pair of brackets or individually bracketed? 
Suggest individually bracketed to simplify the validation process.)

Experimental, Test, or Work in Progress PRs should be opened as Drafts. 

List of tags
(Note: Suggest finding a way to have the list in docs generated from the source of the validation script, 
so it’s automatically updated when the list changes.) 

How to add a new tag to the list
Open an issue(link) to propose a new tag. 

Question, probably for later: what does the flow of a rejected tag in a PR look like? The validation script rejects a tag in a new PR as not a match for any item in the approved list, and…?

[tdcmeehan]

It's possible to reject Git pushes based on some criteria in a scriptable way. I think the mechanics of it, should we choose to valid, is that we can check against a list somewhere in source code, and reject the push, adding a comment for the committer to refer to the development guidelines (and including a link).

[majetideepak]

We should be able to add a bash script in one of the required CI jobs to achieve this. I will work on this.

[majetideepak]

I see we have a bash script for retry here https://github.com/prestodb/presto/blob/master/.github/bin/retry