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

feature: Added CI to verify proper formatting of PR titles. #7470

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

ayushrakesh
Copy link

@ayushrakesh ayushrakesh commented Jun 19, 2024

Added one CI to check that PR titles are properly formatted.

  • I added CHANGELOG entry for this change.
  • Change is not relevant to the end user.

Changes

Added one .yaml file for CI.

Verification

make test

ayushrakesh and others added 6 commits June 15, 2024 20:37
Signed-off-by: ayushrakesh <ayushrakesh.05@gmail.com>
Signed-off-by: ayushrakesh <ayushrakesh.05@gmail.com>
Signed-off-by: ayushrakesh <ayushrakesh.05@gmail.com>
Signed-off-by: ayushrakesh <ayushrakesh.05@gmail.com>
Signed-off-by: ayushrakesh <ayushrakesh.05@gmail.com>
Copy link
Contributor

@douglascamata douglascamata left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The PR title format's still wrong to me.

Plus now we have two source of information regarding how PRs should be written. I would prefer to have one source and link them.

I'm also not sure about how helpful this linting is. It can block a PR from being merged when there are legitimate reasons to break the pattern. It's very easy and fast for any maintainer to edit the title of a PR before merging it.

Curious to see what other maintainers have to say about this.

Comment on lines +18 to +26
PREFIXES="query:|.*:"

if [[ ! "${{ github.event.pull_request.title }}" =~ ^(${PREFIXES}) ]]; then
echo "PR title does not follow the required format: 'prefix: description'"
echo "::error::PR title must start with one of the following prefixes: query:, .*:"
exit 1
else
echo "PR title is valid."
fi
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is still wrong. Please reread the pull request template. The allowed prefix is not only query: and .*: (note that .* here isn't a regex it means "something that applies to all components).

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@douglascamata Okay, means "query" or some single word that applies to all components right? If it is then can you suggest what could be best possible word?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The PR title, as the pull request template says, should start with the name of the Thanos component involved in the PR. We have a list of the components in the official docs (https://thanos.io/tip/components/).

You can also check the history of PRs. You might see some that affected one or more components at the same time. For sure some that affected all of them too.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PREFIXES="query:|compactor:|query-frontend:|receiver:|rule:|sidecar:|store:|tools:"

@douglascamata Is this exactly correct?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're missing the .* (string) prefix, which has to be escaped to not be confused with the meaning that .* has in regexes.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PREFIXES="query:|compactor:|query-frontend:|receiver:|rule:|sidecar:|store:|tools:|\.*:"

Yes, I see that for using .* in bash script as a regex we have to escape it like above. Is this right?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants