Skip to content

Conversation

@ogajduse
Copy link
Member

@ogajduse ogajduse commented Sep 2, 2025

Summary

  • Add exempt_patterns configuration to allow specific commit message patterns to bypass Redmine validation requirements
  • Implement global default patterns applied to all repositories with repository-specific override capability
  • Add regex pattern matching for flexible commit message formats
  • Maintain backward compatibility with existing configuration

Background

Resolves the issue discussed in theforeman/theforeman-rel-eng#519 (comment) where translation/internationalization commits often don't have specific Redmine issues associated with them, causing legitimate commits to fail validation.

Changes

  • Added exempt_patterns field to Config dataclass
  • Updated configuration loading to support global_config section with repository override behavior
  • Implemented is_commit_exempt() function with regex pattern matching
  • Modified commit validation in get_issues_from_pr() to check exemptions before marking commits invalid
  • Added global i18n patterns to repos.yaml for common translation workflows
  • Fixed edge case with empty commit messages in subject property

Global Patterns Added

  • "^i18n - " - Matches "i18n - Update Japanese translations"
  • "^Localization - " - Matches "Localization - Fix German strings"
  • "^Update translations" - Matches "Update translations for 3.5 release"
  • "^Automatic locale update" - Matches "Automatic locale update from Crowdin"

Configuration Examples

# Repository uses global patterns (no local exempt_patterns defined)
theforeman/foreman:
  redmine: foreman
  redmine_required: true
  # Gets global patterns automatically

# Repository completely overrides global patterns  
theforeman/special-repo:
  redmine: special
  redmine_required: true
  exempt_patterns:
    - "^Bot update - "
  # Gets only these patterns (global patterns replaced)

Test plan

  • Manual testing of exempt pattern matching functionality
  • Verification of configuration loading with global and repository-specific patterns
  • Testing edge cases (empty commits, no patterns)
  • Validation that existing behavior remains unchanged for repos without exempt patterns

🤖 Generated with Claude Code

Copy link
Member

@ekohl ekohl left a comment

Choose a reason for hiding this comment

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

I think the whole config is a bit excessive. There should only be a single pattern and I'd be inclined to hardcode it. Then you can use re.compile and reuse the compiled pattern. You can also simplify the code a lot by making it a simple is_exempt method on the Commit class. That keeps rest of the logic easier as well.


def is_commit_exempt(commit: Commit, exempt_patterns: list) -> bool:
"""Check if commit subject matches any exemption pattern."""
import re
Copy link
Member

Choose a reason for hiding this comment

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

Can this be a top level import?

Comment on lines 116 to 119
for pattern in exempt_patterns:
if re.search(pattern, commit.subject, re.IGNORECASE):
return True
return False
Copy link
Member

Choose a reason for hiding this comment

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

Maybe a bit of code golfing

Suggested change
for pattern in exempt_patterns:
if re.search(pattern, commit.subject, re.IGNORECASE):
return True
return False
return any(re.search(pattern, commit.subject, re.IGNORECASE) for pattern in exempt_patterns)

Copy link
Member Author

Choose a reason for hiding this comment

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

I was thinking about that too.

issue_ids.update(commit.refs)
if config.required and not commit.fixes and not commit.refs:
invalid_commits.append(commit)
if not is_commit_exempt(commit, config.exempt_patterns):
Copy link
Member

Choose a reason for hiding this comment

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

Any particular reason you didn't add this to the existing if statement? Would the line become too long?

Copy link
Member Author

Choose a reason for hiding this comment

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

If we had some specific (I do not exactly remember which one) flake ruleset enabled here, it would point out the very same thing. Will fix it.

Comment on lines 14 to 17
- "^i18n - "
- "^Localization - "
- "^Update translations"
- "^Automatic locale update"
Copy link
Member

Choose a reason for hiding this comment

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

I don't like the many variations for this. In our code base we only have 2 and let's keep it that way

Suggested change
- "^i18n - "
- "^Localization - "
- "^Update translations"
- "^Automatic locale update"
- "^i18n - (extracting new, )?pulling from tx$"

Sources:

Copy link
Member Author

Choose a reason for hiding this comment

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

Right, sort of wanted to have a discussion about this - if the i18n one is the only one we want to have in at this moment.

I'll leave only that one there then.

@ogajduse
Copy link
Member Author

ogajduse commented Sep 2, 2025

I've tried to address all your comments. Could you please re-review @ekohl?

Copy link
Member

@ekohl ekohl left a comment

Choose a reason for hiding this comment

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

Thanks! I think this is a lot simpler to understand (and thus maintain).

re.IGNORECASE,
)
COMMIT_ISSUES_REGEX = re.compile(r'#(\d+)')
EXEMPT_COMMIT_REGEX = re.compile(r'^i18n\s*-', re.IGNORECASE)
Copy link
Member

Choose a reason for hiding this comment

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

I think we can be more specific here to keep them standardized. Since they're typically generated by scripts that should be fine.

Suggested change
EXEMPT_COMMIT_REGEX = re.compile(r'^i18n\s*-', re.IGNORECASE)
EXEMPT_COMMIT_REGEX = re.compile(r'^i18n - (extracting new, )?pulling from tx$', re.IGNORECASE)

Copy link
Member Author

Choose a reason for hiding this comment

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

Changed.

…dmine validation

Add simple i18n commit exemption to allow translation commits to bypass
Redmine validation requirements while maintaining validation for regular commits.

Implementation:
- Add hardcoded regex pattern for i18n commits (^i18n\s*- case insensitive)
- Use compiled regex for performance
- Add is_exempt() method to Commit class for clean encapsulation
- Integrate exemption check into existing validation logic

This allows commits like "i18n - Update Japanese translations" to pass
validation without requiring Redmine issue references, addressing the
workflow issue discussed in theforeman/theforeman-rel-eng#519.

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <[email protected]>
@ogajduse ogajduse force-pushed the feature/exempt-i18n-commits branch from b338814 to 17148eb Compare September 12, 2025 13:37
@ogajduse ogajduse requested a review from ekohl September 12, 2025 13:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add support for exempting i18n/translation commits from Redmine validation

2 participants