Conversation
Make sure that the format when specifying the LP bug reference is the one expected by the LP bug update workflow.
alemar99
changed the title
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
[maas-code-reviewer review]
LLM-generated review from https://github.com/canonical/maas-code-reviewer.
Intended to assist a human reviewer, not replace one — suggestions may be
incorrect, please verify before acting.
This correctly implements a verification step to ensure developers specify Launchpad bug references in a way that the lp-bug-update workflow can parse. Using an environment variable (PR_BODY_ENV) for the PR body is a great security practice to prevent command injection.
I did spot a bug with the regular expression that will cause false positives with common English words ending in lp:, as well as a few small details regarding multi-line strings and PCRE sequences.
| PR_BODY="$PR_BODY_ENV" | ||
|
|
||
| # Same keywords as the lp-bug-update workflow. | ||
| if grep -qiP '\b(?:close|closes|closed|fix|fixes|fixed|resolve|resolves|resolved)\b.*LP:\s*' <<< "$PR_BODY"; then |
There was a problem hiding this comment.
Because grep is using the -i (case-insensitive) flag, .*LP:\s* will match any string containing lp:, including common words like help:, gulp:, or scalp:.
For example, if a developer writes Fixes the typo in help:, this first grep will evaluate to true. The inner check on line 172 will then fail (since there is no number after it), causing a false positive that blocks the PR.
To fix this, anchor LP: to a word boundary by changing it to \bLP:\s*. (You should consider applying this fix to the lp-bug-update workflow as well, to prevent it from erroneously extracting a bug ID from something like Fixes the help: 123).
Additionally, note that grep operates line-by-line. This means the closing keyword and LP: must appear on the same line. A multi-line format like:
Fixes the issue.
LP: 12345
will be completely ignored by this check (and consequently by the lp-bug-update workflow). If you intend to require them to be on the same line, this behavior is perfectly fine.
|
|
||
| # Same keywords as the lp-bug-update workflow. | ||
| if grep -qiP '\b(?:close|closes|closed|fix|fixes|fixed|resolve|resolves|resolved)\b.*LP:\s*' <<< "$PR_BODY"; then | ||
| if ! grep -qiP '\b(?:close|closes|closed|fix|fixes|fixed|resolve|resolves|resolved)\b.*LP:\s*\K[0-9]+' <<< "$PR_BODY"; then |
There was a problem hiding this comment.
Two minor points here:
- You should apply the same
\bLP:\s*fix here to keep the regexes identical and prevent false positives. - The
\Kescape sequence (which resets the match start) has no effect here because you are using the-qflag, which suppresses output and only checks for a match's existence. While it's harmless and keeps the pattern identical to thelp-bug-updateworkflow, it is technically unnecessary here.
1 participant
Make sure that the format when specifying the LP bug reference is the one expected by the LP bug update workflow.