Skip to content

chore(ci): GHA cold-cut migration — retire WP/Drone, add GHA caller - #125

Closed
satyamsundaram01 wants to merge 1 commit into
masterfrom
patch/MOEN-gha-migration
Closed

chore(ci): GHA cold-cut migration — retire WP/Drone, add GHA caller#125
satyamsundaram01 wants to merge 1 commit into
masterfrom
patch/MOEN-gha-migration

Conversation

@satyamsundaram01

Copy link
Copy Markdown

Automated GHA migration (SRE). Retires legacy Woodpecker/Drone CI; routes through moengage/github-workflows thin-caller. Cache lock fix already on master. Full deploy chain incl manifest-gen enabled.

…thin-caller

Generated by SRE migration tooling. Retires legacy CI; routes build/test/
deploy/k8s through moengage/github-workflows (pod-local gradle cache fix
already on master). Full deploy chain incl. manifest-gen enabled.
@moe-hodor

moe-hodor Bot commented Jun 4, 2026

Copy link
Copy Markdown

🚪 Hodor is reviewing this PR... 👀

@moe-hodor moe-hodor Bot added branch:patch Weekly maintenance / support tickets (neutral CFR) cfr:neutral Change Failure Rate: neutral impact labels Jun 4, 2026

@moe-hodor moe-hodor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Review: GHA Cold-Cut Migration

Branching & CFR: patch/MOEN-gha-migration — category: patch, impact: Neutral CFR

Summary

This PR retires Drone/Woodpecker CI in favor of GitHub Actions by calling the centralized python-ci.yaml workflow with all build/test/deploy gates disabled. The configuration reduces to secret-scanning only (matching the original WP/Drone behavior).

Quality Gates

PR Size: 26 lines across 3 files — Good size
Commits: Conventional Commits format
Tests: Not applicable (CI config only)
ℹ️ Config: 1 new workflow file
Duplication: None detected

Issues Found

  • 🔴 Critical: 1 (wrong workflow choice for project type)
  • ⚠️ Warning: 2 (semantic mismatch, extra job)
  • 💡 Suggestion: 1 (documentation)

Key Concerns

1. Wrong Workflow for React Native/TypeScript Project

The PR uses python-ci.yaml for a React Native/TypeScript/JavaScript SDK. While the config disables all Python-specific gates (enable_lint: 'false', enable_test: 'false', etc.), this creates semantic confusion and maintenance risk. If someone later enables lint/test thinking "we should enable testing," they'll get Python tooling running against TypeScript code.

Impact: The secret-scan job will work fine (it's language-agnostic), but the workflow choice is architecturally wrong and could cause future incidents.

2. alarm-iac Job Running When It Shouldn't

The alarm-iac job has no enable_* flag and will run unconditionally on PRs and master pushes. This repo has no alarms/ directory and doesn't need infrastructure-as-code alarm management (it's a client SDK, not a deployed service). The job will no-op successfully, but it wastes runner time.

Original behavior: Drone/Woodpecker ONLY ran secret-check + Slack notification. No alarm-iac.

3. Missing Slack Notification on Failure

The original Woodpecker config had:

notify_to_slack:
  when: status: [failure]

The new GHA workflow doesn't include this. Secret-scan failures will be silent unless someone actively monitors the Actions tab.

What's Good

✅ Thin caller pattern matches MoEngage GHA migration strategy
✅ Old CI configs preserved as .Jun04-bkp (audit trail)
✅ Permissions correctly scoped (id-token: write, contents: write, actions: read)
✅ All deploy/build gates correctly disabled for a library repo
✅ Triggers cover all necessary events (push, PR, tags)

Recommendations

Option A (Ideal): Create a minimal secret-check-only.yaml workflow in moengage/github-workflows for SDK/library repos (or use a shared action directly). This avoids the semantic mismatch entirely.

Option B (Pragmatic, if A isn't feasible in the migration timeline): Keep the current config BUT:

  1. Add a comment at the top of ci.yaml explaining why python-ci.yaml is used (secret-scan only, not Python-specific)
  2. Consider adding Slack notification on failure to match original behavior
  3. File a follow-up ticket to migrate to a language-appropriate workflow when available

The current PR will work correctly for secret-scanning (the only active gate), but it's semantically wrong and creates confusion for future maintainers.

Regression Risk

Low — No code changes, only CI config. The secret-scan functionality is preserved. The repository doesn't have tests/builds in CI today (just secret-scan), so the disabled gates don't remove functionality.

Verdict

The configuration is functionally correct but semantically inappropriate. The secret-scan will work, but using python-ci.yaml for a React Native SDK is a code smell. Recommend documenting the rationale or deferring until a proper SDK workflow exists.


Hodor holds the gate. 🚪
1 critical issue (semantic mismatch), 2 warnings (extra job, missing Slack). The gate opens with caution — the PR works but isn't ideal.

Comment thread .github/workflows/ci.yaml
@@ -0,0 +1,26 @@
# Thin caller — minimal (secret-scan only; no build pipeline in source)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Suggestion — Add explanatory comment: Since this is a React Native/TypeScript SDK repo using python-ci.yaml (which is semantically confusing), add a comment explaining the choice:

# Thin caller for GHA migration — uses python-ci.yaml for secret-scan ONLY.
# All Python-specific gates (lint/test/build/deploy) are disabled.
# TODO: Migrate to language-appropriate workflow when SDK/library workflow exists.

This helps future maintainers understand why a Python workflow is used for TypeScript code.

Comment thread .github/workflows/ci.yaml

jobs:
ci:
uses: moengage/github-workflows/.github/workflows/python-ci.yaml@master

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🔴 Critical — Wrong workflow for project type: This repo is a React Native/TypeScript/JavaScript SDK (see package.json files in sdk/core/, sdk/cards/, sdk/inbox/, etc.), but the PR uses python-ci.yaml.

Why this is critical:

  1. Semantic mismatch: Future maintainers might enable enable_lint: 'true' expecting TypeScript linting, but will get Python linting instead (leading to confusing failures or silent no-ops).
  2. Maintenance risk: The workflow is designed for Python services (venv builders, pip, /executor, etc.). While it works today with all gates disabled, it's architecturally wrong.
  3. Discoverability: New team members looking at CI config will be confused why a JS/TS project uses python-ci.

Current state: Works correctly for secret-scan (language-agnostic), but:

  • The alarm-iac job will run unconditionally even though this repo has no alarms (wastes runner time)
  • If anyone later enables lint/test, they'll get Python tools, not Node/TypeScript tools

Options:

  • Ideal: Create sdk-ci.yaml or secret-check-only.yaml in github-workflows for library repos
  • Acceptable: Keep this BUT document prominently (see comment on line 1) and file a follow-up migration ticket

What the original WP/Drone did: ONLY secret-check + Slack notification (no alarm-iac, no Python tooling). The new config adds alarm-iac which isn't needed for a client SDK.

Comment thread .github/workflows/ci.yaml
ci:
uses: moengage/github-workflows/.github/workflows/python-ci.yaml@master
with:
use_gha_venv_path: 'false'

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Warning — use_gha_venv_path not needed: This flag prefixes S3 venv paths with gha/ to avoid WP/GHA collisions during parallel operation. Since this repo:

  1. Is a client SDK (not a deployed service)
  2. Has no venv builders running (all enable_* are false)
  3. Doesn't produce artifacts uploaded to S3

...this flag has no effect. It's harmless but adds noise. You can set it to 'true' or 'false' — neither matters since no venv jobs will run.

Recommendation: Leave as-is (setting to false is fine) or remove the line entirely (the workflow default is 'false').

@moe-hodor moe-hodor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Additional Inline Comments Posted

Issues (3 after filtering):
🔴 Critical: 1
⚠️ Warning: 1
💡 Suggestion: 1

Hodor filtered 1 low-signal comment(s) to reduce noise.
Verdict: COMMENT

@satyamsundaram01

Copy link
Copy Markdown
Author

Skipped - Covered by Wiz (org-wide secret/security scanning already enabled; this repo's Woodpecker/Drone pipeline was secret-scanning only, so a GHA migration adds no coverage).

@satyamsundaram01
satyamsundaram01 deleted the patch/MOEN-gha-migration branch June 5, 2026 12:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

branch:patch Weekly maintenance / support tickets (neutral CFR) cfr:neutral Change Failure Rate: neutral impact

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant