skara icon indicating copy to clipboard operation
skara copied to clipboard

2170: Warn on trailing period in PR titles

Open zhaosongzs opened this issue 10 months ago • 7 comments

This patch is trying to add a new jcheck called "issuestitle". This jcheck would check if the issue's title has a trailing period or leading lowercase letter. And we would like to configure "issuestitle" jcheck as warning in the repos since in some cases, trailing periods or leading lowercase letter are valid.

When implementing this, I found that although we can configure a jcheck as warning in the jcheck configuration file(.jcheck/conf), we fail to differentiate between jchecks as error and jchecks as warning. When jchecks fail as warnings, they cause the local jcheck to exit with a status code of 1. Also, they will be integration blockers when Skara bots evaluate pull requests. Therefore, in this patch, I also make Skara CLI and Skara bots be able to handle jcheck warnings properly.


Progress

  • [ ] Change must be properly reviewed (1 review required, with at least 1 Reviewer)
  • [x] Change must not contain extraneous whitespace

Issues

  • SKARA-2170: Warn on trailing period in PR titles (Enhancement - P4)
  • SKARA-2248: Warn on leading lowercase letter in PR titles (Enhancement - P4)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/skara.git pull/1638/head:pull/1638
$ git checkout pull/1638

Update a local copy of the PR:
$ git checkout pull/1638
$ git pull https://git.openjdk.org/skara.git pull/1638/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 1638

View PR using the GUI difftool:
$ git pr show -t 1638

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/skara/pull/1638.diff

Webrev

Link to Webrev Comment

zhaosongzs avatar Apr 15 '24 22:04 zhaosongzs

:wave: Welcome back zsong! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

bridgekeeper[bot] avatar Apr 15 '24 22:04 bridgekeeper[bot]

@zhaosongzs This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

:mag: One or more changes in this pull request modifies files in areas of the source code that often require two reviewers. Please consider if this is the case for this pull request, and if so, await a second reviewer to approve this pull request before you integrate it.

After integration, the commit message for the final commit will be:

2170: Warn on trailing period in PR titles
2248: Warn on leading lowercase letter in PR titles

Reviewed-by: erikj

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 10 new commits pushed to the master branch:

  • 53f65c291fb28d2b5f9c6244d4d7f9319500587f: 2249: serviceability mailing list should be notified of changes to heapdumper and diagnostic files
  • 695dbc6b9603b5c1b4237ce24d4aacf9d1461d1d: 2247: When pr's headHash is missing, stop further processing
  • d8a8a24d35b1ee461e42364d8d6c2a1215a8ca8f: 2246: Check if the pull request is still open before issuing "integrate" automatically
  • 7e810cac854f2f55cc7fb85f76b4ebc6cabaf2c6: 2234: /reviewers N should remove ready status for merge pull requests
  • 052f7e0132a18f369164ca5c613df3be93eb80d4: 2226: Make it possible to avoid "forward backports"
  • 2372cbbcc5d5b66715b8e0469b6ed798404435c8: 2245: Don't check security in JiraIssueTrackerFactory
  • a13dcde9bc5f9c3575a6839957ee483ac00d7cc3: 2244: SecurityLevel in JiraHost is useless
  • ac3c81f05fbe2d7ef5b690ca0819a3e1cb96340e: 2242: Creating pull request on github assumes source is personal fork of user
  • 1dfc1aea12e7f334af142bbddc1f5e9db61ce38a: 2233: TestHost does not set assignees correctly for backports
  • c69015f7fc0b63d6c121ebbd743ca39e15ab224e: 2239: Stop logging "issue: bad whitespace"

Please see this link for an up-to-date comparison between the source branch of this pull request and the master branch. As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

openjdk[bot] avatar Apr 15 '24 22:04 openjdk[bot]

I looked at the code to try to answer my own question, but could not find out an easy answer, so I'll say it out loud instead:

How do you make it so that this is a warning only, and not a blocking error?

magicus avatar Apr 16 '24 08:04 magicus

I looked at the code to try to answer my own question, but could not find out an easy answer, so I'll say it out loud instead:

How do you make it so that this is a warning only, and not a blocking error?

Hi Magnus, I haven't finished this patch... So it's still in the draft state. I will finish it today.

zhaosongzs avatar Apr 16 '24 15:04 zhaosongzs

Webrevs

mlbridge[bot] avatar Apr 17 '24 18:04 mlbridge[bot]

/issue add SKARA-2248

zhaosongzs avatar Apr 30 '24 21:04 zhaosongzs

@zhaosongzs Adding additional issue to issue list: 2248: Warn on leading lowercase letter in PR titles.

openjdk[bot] avatar Apr 30 '24 21:04 openjdk[bot]

This PR is ready for review. Could someone please review it? ping...

zhaosongzs avatar May 06 '24 17:05 zhaosongzs

This PR is ready for review. Could someone please review it? ping...

Sorry, I saw your comment about it not being ready so assumed you would say something when it was. Noticed the draft state change now.

erikj79 avatar May 06 '24 17:05 erikj79

Thanks for the review! /integrate

zhaosongzs avatar May 07 '24 16:05 zhaosongzs

Going to push as commit 2edf2280cbc800a7e578e134d68fe9b33f8e1bd7. Since your change was applied there have been 10 commits pushed to the master branch:

  • 53f65c291fb28d2b5f9c6244d4d7f9319500587f: 2249: serviceability mailing list should be notified of changes to heapdumper and diagnostic files
  • 695dbc6b9603b5c1b4237ce24d4aacf9d1461d1d: 2247: When pr's headHash is missing, stop further processing
  • d8a8a24d35b1ee461e42364d8d6c2a1215a8ca8f: 2246: Check if the pull request is still open before issuing "integrate" automatically
  • 7e810cac854f2f55cc7fb85f76b4ebc6cabaf2c6: 2234: /reviewers N should remove ready status for merge pull requests
  • 052f7e0132a18f369164ca5c613df3be93eb80d4: 2226: Make it possible to avoid "forward backports"
  • 2372cbbcc5d5b66715b8e0469b6ed798404435c8: 2245: Don't check security in JiraIssueTrackerFactory
  • a13dcde9bc5f9c3575a6839957ee483ac00d7cc3: 2244: SecurityLevel in JiraHost is useless
  • ac3c81f05fbe2d7ef5b690ca0819a3e1cb96340e: 2242: Creating pull request on github assumes source is personal fork of user
  • 1dfc1aea12e7f334af142bbddc1f5e9db61ce38a: 2233: TestHost does not set assignees correctly for backports
  • c69015f7fc0b63d6c121ebbd743ca39e15ab224e: 2239: Stop logging "issue: bad whitespace"

Your commit was automatically rebased without conflicts.

openjdk[bot] avatar May 07 '24 16:05 openjdk[bot]

@zhaosongzs Pushed as commit 2edf2280cbc800a7e578e134d68fe9b33f8e1bd7.

:bulb: You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

openjdk[bot] avatar May 07 '24 16:05 openjdk[bot]