-
Notifications
You must be signed in to change notification settings - Fork 4
chore/parameterise platform environment #248
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
Conversation
|
Claude finished @olivermeyer's task —— View job PR Review: Parameterize Platform Environment ✅SummaryThis PR adds 🟢 Review Findings: APPROVEDThis is a well-structured change that improves flexibility while maintaining safety. All checks pass. ✅ Correctness Verification1. Workflow Dispatch Configuration ✅workflow_dispatch:
inputs:
platform_environment:
description: 'Environment to test against'
required: false
default: 'staging'
type: choice
options:
- staging
- productionAnalysis:
2. Parameterization Change ✅Before: platform_environment: "production" # ❌ HardcodedAfter: platform_environment: ${{ inputs.platform_environment || 'staging' }} # ✅ Dynamic with safe fallbackAnalysis:
3. Impact on _test.yml ✅The inputs:
platform_environment:
description: 'Environment to test against, that is staging or production'
required: false
default: 'staging'
type: stringAnalysis:
🎯 Behavior ChangesCRITICAL: This is a Breaking Change (But Correct)Previous behavior:
New behavior:
Why This Change is CORRECTAccording to
The previous hardcoded What Was Wrong BeforeThe old configuration had: platform_environment: "production" # ❌ WRONG - PRs should use stagingThis meant every PR was testing against the production API, which is explicitly forbidden by the CI/CD documentation:
📋 Safety Analysis1. No Breaking Changes to Existing Workflows ✅Automatic triggers (push, PR, release, tag):
Manual triggers (workflow_dispatch):
2. Environment Secrets ✅Both environments have their secrets passed to secrets:
AIGNOSTICS_CLIENT_ID_DEVICE_STAGING: ${{ secrets.AIGNOSTICS_CLIENT_ID_DEVICE_STAGING }}
AIGNOSTICS_REFRESH_TOKEN_STAGING: ${{ secrets.AIGNOSTICS_REFRESH_TOKEN_STAGING }}
GCP_CREDENTIALS_STAGING: ${{ secrets.GCP_CREDENTIALS_STAGING }}
AIGNOSTICS_CLIENT_ID_DEVICE_PRODUCTION: ${{ secrets.AIGNOSTICS_CLIENT_ID_DEVICE_PRODUCTION }}
AIGNOSTICS_REFRESH_TOKEN_PRODUCTION: ${{ secrets.AIGNOSTICS_REFRESH_TOKEN_PRODUCTION }}
GCP_CREDENTIALS_PRODUCTION: ${{ secrets.GCP_CREDENTIALS_PRODUCTION }}Analysis:
3. Alignment with Scheduled Tests ✅This change aligns with the existing scheduled test workflows:
Now the main CI/CD can also support both environments via manual trigger. 🔍 Additional Considerations1. Documentation Alignment ✅This change perfectly aligns with
2. No CI/CD Documentation Updates Needed ✅The PR description states:
This is accurate. The 3. Testing the Change
|
| Category | Status | Notes |
|---|---|---|
| Syntax | ✅ Pass | Valid YAML, correct GitHub Actions syntax |
| Logic | ✅ Pass | Fallback chain works correctly |
| Safety | ✅ Pass | Defaults to safe environment (staging) |
| Documentation | ✅ Pass | Aligns with (and fixes) documented behavior |
| Breaking Changes | Fixes incorrect production usage in PRs | |
| Security | ✅ Pass | No security concerns |
| Testing | ✅ Pass | This PR will test the new behavior |
✅ Recommendation: APPROVE AND MERGE
This PR:
- ✅ Fixes a critical misalignment where PRs tested against production (should be staging)
- ✅ Adds useful flexibility for manual workflow triggers
- ✅ Maintains backward compatibility for automatic triggers
- ✅ Follows GitHub Actions best practices
- ✅ Aligns with documented CI/CD strategy
No changes requested. This is a quality improvement that makes the CI/CD more flexible and corrects a previous misconfiguration.
🚀 Next Steps
- ✅ Merge this PR
- ✅ Monitor this PR's CI run to confirm tests use staging
- ✅ Optionally test manual workflow_dispatch after merge
⚠️ Consider notifying team about the environment change (PRs now correctly use staging instead of production)
Great work, @olivermeyer! This improves both the flexibility and correctness of the CI/CD pipeline. 👏
❌ 2 Tests Failed:
View the full list of 2 ❄️ flaky test(s)
To view more test analytics, go to the Test Analytics Dashboard |
|
Claude finished @olivermeyer's task —— View job PR Review: Parameterize Platform Environment
|
| Trigger Type | inputs.platform_environment |
Resolved Value | Correct? |
|---|---|---|---|
| PR (opened/sync) | null |
'staging' |
✅ YES |
| Push to main | null |
'staging' |
✅ YES |
Tag v*.*.* |
null |
'staging' |
✅ YES |
| Release created | null |
'staging' |
✅ YES |
| Manual (default) | 'staging' |
'staging' |
✅ YES |
| Manual (select prod) | 'production' |
'production' |
✅ YES (intentional) |
Key Insight: This is a BREAKING CHANGE but in the RIGHT DIRECTION - it corrects previously incorrect behavior.
5. Security & Safety ✅
Credential Handling:
- ✅ Both staging and production secrets already passed to
_test.yml(lines 105-110) - ✅
_test.ymlselects appropriate credentials based on environment parameter - ✅ No new secret exposure
Environment Selection:
- ✅
type: choiceprevents arbitrary environment strings - ✅ Cannot accidentally use invalid environment
- ✅ Manual production access requires explicit selection
- ✅ Default to staging is safest option
Risk Assessment:
⚠️ Medium Risk: Changes test environment for all existing triggers- ✅ Mitigated: New environment (staging) is safer than old (production)
- ✅ Intentional: Aligns with documented strategy
- ✅ Reversible: Can be quickly fixed if issues arise
6. Conventional Commits Compliance ✅
Commit Messages:
9286acb: chore: parameterize platform_environment in CI/CD workflow
f59390f: chore(gha): Target staging with CI/CD workflow
- ✅ Uses valid type:
chore - ✅ Optional scope:
(gha)for "GitHub Actions" - ✅ Descriptive commit messages
- ✅ Follows repository convention
7. Testing Strategy Alignment ✅
No test marker issues: ✅ (No test code changed)
Test execution:
- This PR's tests will validate the new behavior
- Current PR CI run is using the new configuration
- Verify in logs:
AIGNOSTICS_PLATFORM_ENVIRONMENT=staging
Coverage: ✅ No source code changed, coverage requirements N/A
8. CI/CD Impact Assessment
Workflows Affected:
- ✅
ci-cd.yml- Modified (this PR) - ✅
_test.yml- No changes needed (already compatible) - ✅
test-scheduled-staging.yml- No changes needed (explicitly passes'staging') - ✅
test-scheduled-production.yml- No changes needed (explicitly passes'production')
Behavior Matrix:
| Workflow | Before | After | Impact |
|---|---|---|---|
| PR CI | Production ❌ | Staging ✅ | FIXED |
| Push to main | Production ❌ | Staging ✅ | FIXED |
| Tag release | Production ❌ | Staging ✅ | IMPROVED |
| Manual trigger | N/A | Staging (default) ✅ | NEW FEATURE |
| Scheduled staging | Staging ✅ | Staging ✅ | NO CHANGE |
| Scheduled production | Production ✅ | Production ✅ | NO CHANGE |
BetterStack Monitoring: ✅ No impact (scheduled tests unchanged)
💡 Observations & Recommendations
Strengths 👏
- Fixes Critical Issue: Corrects production usage in PRs (violated policy)
- Adds Flexibility: Manual trigger capability for production testing when needed
- Safe Defaults: Double fallback to staging (caller + callee)
- Type Safety:
type: choiceprevents invalid values - Backward Compatible: Existing scheduled workflows unaffected
- Well Documented: PR description accurately describes changes
Potential Considerations (Non-Blocking)
1. Documentation Update (Optional)
Consider adding a note to .github/CLAUDE.md about the manual trigger capability.
Suggested Addition (around line 1031, after "### Manual Testing with Claude"):
### Manual CI/CD Workflow Runs
You can manually trigger the main CI/CD pipeline with a custom environment:
**Via GitHub UI**:
1. Actions → CI/CD → Run workflow
2. Select branch
3. Choose environment: `staging` (default) or `production`
**Via GitHub CLI**:
```bash
# Run with staging (default)
gh workflow run ci-cd.yml --ref my-branch
# Run with production
gh workflow run ci-cd.yml --ref my-branch -f platform_environment=productionUse Cases:
- Testing specific branch against production (use with caution)
- Validating fix against production environment
- Release candidate validation
**Decision**: Not required for this PR - can be added in follow-up.
##### 2. Add Safety Check for PR + Production (Future Enhancement)
Consider adding a validation step to prevent manual production testing in PR context:
```yaml
- name: Validate environment for PR
if: github.event_name == 'pull_request' && inputs.platform_environment == 'production'
run: |
echo "::error::Production environment is not allowed for PR testing"
exit 1
Decision: Not necessary now - the default mechanism already prevents this in normal flows. Only manual override can select production, which requires intentional action.
3. Team Communication (Important)
Recommendation: After merge, consider notifying the team about this change:
Heads up: PR #248 changes CI/CD to test against staging (was previously production). This aligns with our documented policy. If you notice any test behavior changes, this is expected.
Why: Developers may have unknowingly relied on production testing in PRs. Setting expectations prevents confusion.
🚦 Final Assessment
| Category | Status | Notes |
|---|---|---|
| Syntax | ✅ Pass | Valid GitHub Actions YAML |
| Logic | ✅ Pass | Fallback chain works correctly |
| Safety | ✅ Pass | Defaults to safer environment |
| Documentation Alignment | ✅ Pass | Fixes documented policy violation |
| Breaking Changes | But GOOD - fixes incorrect behavior | |
| Security | ✅ Pass | No new security concerns |
| Testing | ✅ Pass | This PR validates the change |
| Commit Convention | ✅ Pass | Follows conventional commits |
| CI/CD Best Practices | ✅ Pass | Follows GitHub Actions patterns |
✅ Recommendation: APPROVED
This PR:
- ✅ Fixes a critical misalignment where PRs incorrectly tested against production
- ✅ Adds useful manual trigger capability
- ✅ Maintains backward compatibility for scheduled tests
- ✅ Follows GitHub Actions best practices
- ✅ Aligns with documented CI/CD strategy
- ✅ Uses safe defaults and type validation
No blocking issues found. This is a quality improvement that corrects a previous misconfiguration.
🚀 Next Steps
After Merge:
- ✅ Monitor this PR's CI run to confirm staging usage (logs should show
AIGNOSTICS_PLATFORM_ENVIRONMENT=staging) - ✅ Optionally test manual
workflow_dispatchwith both environments ⚠️ Consider team notification about environment change- 📝 Optional: Add manual trigger documentation to
.github/CLAUDE.md(future PR)
Verification Commands (after merge):
# Verify staging is used in CI logs
gh run view <run-id> --log | grep AIGNOSTICS_PLATFORM_ENVIRONMENT
# Test manual trigger with staging (default)
gh workflow run ci-cd.yml --ref main
# Test manual trigger with production (careful!)
gh workflow run ci-cd.yml --ref main -f platform_environment=productionGreat work, @olivermeyer! This PR improves both flexibility and correctness of the CI/CD pipeline. 👏
The previous bot review was thorough and accurate - I concur with its assessment. This change is exactly what's needed to align implementation with documented best practices for medical device software CI/CD.
|




platform_environment; defaults tostaging