Skip to content

feat: Add environment variable branch support and enhanced pattern ma… #844

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

Open
wants to merge 3 commits into
base: main-enterprise
Choose a base branch
from

Conversation

jamengual
Copy link

This pull request introduces several updates to improve code robustness, handle edge cases, and enhance functionality. The most significant changes include better handling of branch references, improved regex detection in glob patterns, and enhanced error handling and fallback mechanisms in the Settings class.

Branch Reference Handling:

  • index.js: Added logic to dynamically determine and validate branch references using environment variables, with fallback to the main branch if undefined.

Glob Pattern Matching:

  • lib/glob.js: Introduced regex detection for patterns and fallback to glob matching if regex parsing fails. Added a helper method isRegexPattern to identify regex-specific characters.

Enhanced Error Handling and Fallbacks:

  • lib/settings.js: Updated Settings class to handle optional chaining for installation_id and repository owner/repo fields, avoiding undefined errors. [1] [2] [3] [4]
  • lib/settings.js: Added checks to ensure PR comment and check run updates are only performed in webhook context.
  • lib/settings.js: Improved logic for creating PR comments by verifying the presence of pull requests in the webhook payload.

…tching - Add support for environment variables in syncInstallation (SAFE_SETTINGS_BRANCH, GITHUB_HEAD_REF, GITHUB_REF_NAME, GITHUB_REF) - Enhance Glob class to support both regex and glob patterns with intelligent detection - Fix backwards compatibility issues for GitHub Actions environment: Handle undefined payload.installation gracefully, Only update check runs/PR comments when in webhook context, Add defensive checks for undefined repo properties, Handle both string and object repository owner formats These changes enable safe-settings to work properly in GitHub Actions while maintaining full backwards compatibility.
@Copilot Copilot AI review requested due to automatic review settings June 5, 2025 23:00
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR adds support for configurable branch references via environment variables, improves glob pattern handling by detecting regex usage and falling back on glob matching, and hardens the Settings class with safer defaults and scoped webhook updates.

  • Branch Reference Handling: derive and normalize ref from various environment variables, defaulting to main.
  • Glob Matching: detect regex-like patterns in Glob.test, try a RegExp, and fallback to minimatch if parsing fails.
  • Settings Fallbacks: use optional chaining for installation and repo IDs, guard PR comment/check-run code to webhook context.

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File Description
index.js Added logic to compute ref from env vars, normalize it, log, and pass to syncAllSettings
lib/glob.js Added isRegexPattern helper; updated test to try regex then fallback to glob
lib/settings.js Made installation_id and repo fields null-safe, scoped PR comment/check-run updates to webhook payload, and handled mixed owner formats
Comments suppressed due to low confidence (4)

lib/glob.js:11

  • Consider adding unit tests for both regex paths and glob fallback, including invalid regex patterns that trigger the fallback branch.
if (this.isRegexPattern(this.pattern)) {

index.js:239

  • Add tests for the branch-detection logic covering scenarios with different environment variable combinations and refs/ prefixes.
let ref = process.env.SAFE_SETTINGS_BRANCH || process.env.GITHUB_HEAD_REF || process.env.GITHUB_REF_NAME || process.env.GITHUB_REF

index.js:238

  • [nitpick] It might help to explicitly log or document which branch is assumed when no ref env vars are set (e.g., defaulting to main).
// Use environment variable for branch reference, fallback to undefined (main branch)

lib/settings.js:172

  • The variable payload is not defined in this scope. Use this.context.payload or destructure const { payload } = this.context before referencing it.
if (!payload || !payload.check_run) {

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

Successfully merging this pull request may close these issues.

1 participant