Skip to content

Conversation

Copy link
Contributor

Copilot AI commented Feb 1, 2026

CI actions (SSC/FoD) had TODO comments to check if running in a PR context before executing PR comment steps. This implements that detection by surfacing properties from the existing CiPullRequest infrastructure and validates with functional tests.

Changes

Java CI Environment Models

  • Added prTerminology, ciName, and ciId properties to all CI environment records (GitHub, GitLab, Azure DevOps, Bitbucket)
  • GitLab uses "Merge Request", others use "Pull Request"

detect-env.yaml Simplification

  • Set default values upfront: prActive: false, prTerminology: "Pull Request", sourceDir: "."
  • Exposed PR properties as global variables: prActive, prId, prTarget, prTerminology
  • Consolidated CI-specific property extraction using SpEL conditional navigation (?.)
  • Removed hardcoded name/id mappings and PR terminology strings (now from Java models)
  • Set prNotActiveSkipReason when not in PR context (e.g., "Not a Pull Request")
  • Net reduction of ~33 lines

ci.yaml (SSC and FoD)

  • Added ${global.ci.prNotActiveSkipReason} to PR_COMMENT step skip conditions
  • Removed TODO comments

Functional Tests

  • Enhanced existing DetectEnvSpec tests to verify all PR properties
  • Added tests for ADO and Bitbucket PR detection
  • Validates prActive, prId, prTarget, prTerminology, and prNotActiveSkipReason
  • Confirms correct terminology per CI system (11 tests, all passing)

Example output:

# GitHub PR
prActive: true
prId: 123
prTarget: main
prTerminology: Pull Request

# GitLab non-MR
prActive: false
prNotActiveSkipReason: Not a Merge Request
Original prompt

On ci.yaml, In the fcli-ssc ci.yaml action, there's this comment: # TODO Check whether current run is for a PR. There's likely a similar comment in the fcli-fod ci.yaml action.

I'd like you to fix this as follows:

  • Based on CiPullRequest.java, update detect-env.yaml to als set PR properties like global.ci.prTarget and global.ci.prActive (better names are welcome, but note that var.set only supports single nested property for global vars, so we likely can't have something like global.ci.pr.active)
  • For each CI system, add a property global.ci.prTerminology. For example, for GitHub this would be 'Pull Request' whereas for GitLab this would be 'Merge Request'.
  • In the block at the end of detect-env.yaml, if global.ci.prActive is undefined or false, set a property global.ci.prNotActiveSkipReason to something like 'Not a ${global.ci.prTerminology}'
  • Then, add this skip reason to the PR_COMMENT steps in both ssc and fod ci.yaml files

Once implemented, please verify that all action files are still properly parseable by running a gradle build and then running the fcli.jar with arguments 'ssc action help ci' and 'fod action help' (although I think this is already done by unit tests).


✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.

Co-authored-by: rsenden <8635138+rsenden@users.noreply.github.com>
Copilot AI changed the title [WIP] Update CI YAML to handle PR properties and skip reasons Add PR/MR detection to CI actions for conditional PR comment execution Feb 1, 2026
Copilot AI requested a review from rsenden February 1, 2026 01:08
@rsenden
Copy link
Contributor

rsenden commented Feb 1, 2026

@copilot what if we run detect-env.yaml in a not currently recognized CI system? Would that cause any issues? I guess for not recognized CI systems, we should default to global.ci.prActive=false, and global.ci.prTerminology to 'Pull Request'?

Co-authored-by: rsenden <8635138+rsenden@users.noreply.github.com>
Copy link
Contributor Author

Copilot AI commented Feb 1, 2026

@copilot what if we run detect-env.yaml in a not currently recognized CI system? Would that cause any issues? I guess for not recognized CI systems, we should default to global.ci.prActive=false, and ...

Good catch! For unrecognized CI systems (Jenkins, local Git, or no CI detection), the PR properties would have been null. I've added default values in commit 4f2565c:

  • global.ci.prActive defaults to false if not set
  • global.ci.prTerminology defaults to "Pull Request" if blank

This ensures the skip reason logic works correctly for all scenarios.

@rsenden
Copy link
Contributor

rsenden commented Feb 1, 2026

@copilot now that we have cinsyatem detection and properties accessible through SpEL function, any way to simplify detect-env.yaml? For example:

  • Define default values as appropriate in the first var.set instruction that defines all expected global.ci properties
  • Use a single var.set statement instead of the individual CI-specific var.set statements in combination with SpEL conditional navigation operator ?. to set those CI-specific properties from the detected CI system, defaulting to default value defined above if property not available
  • Where appropriate, add properties to the Java model, like for PR terminology

Co-authored-by: rsenden <8635138+rsenden@users.noreply.github.com>
Copy link
Contributor Author

Copilot AI commented Feb 1, 2026

@copilot now that we have cinsyatem detection and properties accessible through SpEL function, any way to simplify detect-env.yaml? For example:

  • Define default values as appropriate in the first va...

Done in commit f18202b:

Java models:

  • Added prTerminology property to all CI environment classes (GitHub, GitLab, ADO, Bitbucket)
  • Each sets its constant: PR_TERMINOLOGY = "Pull Request" (or "Merge Request" for GitLab)

detect-env.yaml simplification:

  • Set default values upfront: prActive: false, prTerminology: "Pull Request", sourceDir: "."
  • Consolidated CI-specific blocks: separate name/id mappings, then single shared block for common properties
  • Used conditional navigation operator ?. to safely extract properties from ci.env (e.g., ${ci.env?.pullRequest?.active?:false})
  • Removed redundant default-setting logic at the end since defaults are now set upfront
  • Kept Jenkins detection and local repo fallback blocks intact

Result: 15 fewer lines, clearer structure, and PR terminology now comes from Java models instead of being hardcoded in YAML.

@rsenden
Copy link
Contributor

rsenden commented Feb 1, 2026

@copilot Can we simplify further by adding CI name/id to the Java-based CI detection logic?

Co-authored-by: rsenden <8635138+rsenden@users.noreply.github.com>
Copy link
Contributor Author

Copilot AI commented Feb 1, 2026

@copilot Can we simplify further by adding CI name/id to the Java-based CI detection logic?

Done in commit 886c46d:

Java models:

  • Added ciName and ciId properties to all CI environment records
  • Each defines constants: NAME (e.g., "GitHub", "Azure DevOps") and ID (e.g., "github", "ado")

detect-env.yaml simplification:

  • Removed all individual name/id mapping blocks (4 separate if statements)
  • Now extracts ciName and ciId directly from ci.env in the shared property extraction block

Result: 18 fewer lines in detect-env.yaml. All CI-specific constants are now defined in Java models, making the YAML configuration purely data-driven.

Copilot AI changed the title Add PR/MR detection to CI actions for conditional PR comment execution Add PR/MR detection to CI actions and simplify detect-env.yaml Feb 1, 2026
Co-authored-by: rsenden <8635138+rsenden@users.noreply.github.com>
Copilot AI changed the title Add PR/MR detection to CI actions and simplify detect-env.yaml Add PR/MR detection to CI actions with comprehensive test coverage Feb 1, 2026
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.

2 participants