Skip to content

Add configurable config profile override for staging and shared master#435

Merged
jfrench9 merged 3 commits into
mainfrom
feature/staging-configurable-profile
Mar 7, 2026
Merged

Add configurable config profile override for staging and shared master#435
jfrench9 merged 3 commits into
mainfrom
feature/staging-configurable-profile

Conversation

@jfrench9
Copy link
Copy Markdown
Member

@jfrench9 jfrench9 commented Mar 7, 2026

Summary

Introduces support for overriding the configuration profile used in staging and shared master deployments. This allows deployments to target different configuration profiles without requiring code changes, enabling more flexible environment management.

Key Accomplishments

  • Workflow parameter support: Added a config_profile input/parameter to the deploy-graph.yml and deploy-graph-ladybug.yml GitHub Actions workflows, allowing the configuration profile to be specified at deploy time.
  • Staging workflow integration: Wired the new config profile parameter through the staging.yml workflow so it propagates correctly during staging deployments.
  • CloudFormation support: Updated the graph-ladybug.yaml CloudFormation template to accept and pass through the config profile override as a parameter.
  • Runtime profile resolution: Extended graph_tier.py to read and apply the config profile override, enabling the application to resolve the correct configuration at runtime based on the provided profile.

Breaking Changes

None. The config profile override is additive — existing deployments that do not specify a profile will continue to use the default behavior.

Testing Notes

  • Verify that staging deployments work correctly without specifying a config profile override (default behavior preserved).
  • Verify that staging deployments correctly pick up and apply a custom config profile when one is explicitly provided.
  • Confirm that the CloudFormation stack deploys successfully with the new parameter, both when the parameter is provided and when it is omitted/defaulted.
  • Validate that the graph tier runtime correctly resolves configuration based on the overridden profile.

Infrastructure Considerations

  • A new parameter has been added to the CloudFormation template — existing stacks may need a parameter update on the next deployment cycle.
  • CI/CD workflows now accept an additional optional input; ensure any upstream workflow triggers or automation that invoke these workflows are compatible with the new parameter.
  • No new infrastructure resources are created; this change only affects configuration resolution and parameterization.

🤖 Generated with Claude Code

Branch Info:

  • Source: feature/staging-configurable-profile
  • Target: main
  • Type: feature

Co-Authored-By: Claude noreply@anthropic.com

jfrench9 added 2 commits March 6, 2026 21:40
- Introduced `config_profile` input in `deploy-graph-ladybug.yml` and `lbug_shared_config_profile` in `deploy-graph.yml` to allow for configuration profile overrides.
- Updated `staging.yml` to include the new shared config profile variable.
- Enhanced `graph-ladybug.yaml` to accept a `ConfigProfile` parameter for staging deployments.
- Modified `graph_tier.py` to utilize the config profile for shared master deployments, enabling staging to use production configurations.
@jfrench9
Copy link
Copy Markdown
Member Author

jfrench9 commented Mar 7, 2026

@claude please review this PR

@claude
Copy link
Copy Markdown

claude Bot commented Mar 7, 2026

Claude finished @jfrench9's task in 1m 25s —— View job


PR Review: Add configurable config profile override for staging and shared master

  • Gather context and understand the request
  • Read changed files and analyze the implementation
  • Post comprehensive review feedback

Overall the PR is clean and the approach is well-designed — additive, backward-compatible, and logically consistent. A few issues worth addressing before merge.


Issues

1. os.getenv() directly — violates repo policy (graph_tier.py:103)

The new code calls os.getenv("LBUG_CONFIG_PROFILE") directly, which violates the rule in CLAUDE.md:

Never use os.getenv() directly — use centralized config: from robosystems.config import env

LBUG_CONFIG_PROFILE should be declared in robosystems/config/env.py and accessed via env.LBUG_CONFIG_PROFILE. There's already a pre-existing violation on line 68 (os.getenv("DEBUG", ...)), but that doesn't make it okay to add more.

Fix this →

2. Shell injection risk in deploy-graph.yml:404

writer=$(yq eval -o=json ".${CONFIG_PROFILE}.writers[] | select(.name == \"ladybug-shared\")" .github/configs/graph.yml)

CONFIG_PROFILE is derived from ${{ inputs.lbug_shared_config_profile }} — a GitHub Actions workflow input — and interpolated directly into a shell command. If the input contains shell metacharacters or a yq path injection string (e.g., production | . as $x |), it could break parsing or yield unexpected output. This is relatively low risk since the input comes from a trusted workflow caller, but it's worth defending against. Consider validating the value against an allowlist (e.g., production|staging|development) before using it in the yq expression.

Fix this →


Minor Notes

3. Inconsistency: CFN sets env var for all tiers, Python only applies it for shared

In graph-ladybug.yaml:448, LBUG_CONFIG_PROFILE is written to /etc/environment whenever ConfigProfile is non-empty, regardless of WriterTier. But in graph_tier.py:104, the override only takes effect when tier == GraphTier.LADYBUG_SHARED.value. This means a non-shared writer that gets a ConfigProfile set will have a silently ignored env var. The behavior is intentional (per the PR), but it could confuse a future operator. Consider either restricting the CFN env var to shared tiers (similar to the SHARED_REPOSITORIES guard on line 447) or adding a comment to graph_tier.py noting the intentional scope restriction.

4. Redundant || '' in staging.yml:457

lbug_shared_config_profile: ${{ vars.LBUG_SHARED_CONFIG_PROFILE_STAGING || '' }}

When a GitHub Actions repo variable is unset, the expression already evaluates to an empty string. The || '' fallback is redundant and inconsistent with the other parameters on adjacent lines (e.g., lbug_shared_min_instances: ${{ vars.LBUG_SHARED_MIN_INSTANCES_STAGING }}). It works fine either way, but worth standardizing.


Summary

# Severity File Issue
1 Medium graph_tier.py:103 os.getenv() violates centralized config rule
2 Low-Medium deploy-graph.yml:404 Unsanitized input in yq shell command
3 Low graph-ladybug.yaml:448 / graph_tier.py:104 Inconsistency between where env var is set vs applied
4 Nit staging.yml:457 Redundant `

The architecture and overall approach are solid. Issue #1 is the main one to fix before merge given the explicit repo convention.
|

- Added validation for the config profile in the deploy-graph workflow to ensure it matches known environments (production, staging, development).
- Updated the setup script to set the default config profile for staging to "staging".
- Adjusted the CloudFormation template to conditionally set the config profile based on the writer tier.
- Introduced a new environment variable for the config profile in the EnvConfig class to support dynamic configuration.
@jfrench9 jfrench9 merged commit ba73280 into main Mar 7, 2026
7 checks passed
@jfrench9 jfrench9 deleted the feature/staging-configurable-profile branch March 10, 2026 04:21
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