Skip to content

Conversation

@emsearcy
Copy link
Contributor

Summary

This PR improves the Helm chart deployment documentation and configuration for the v1-sync-helper service.

Changes

Installation Documentation

  • Added comprehensive installation sections for both local and OCI image deployments
  • Included required --set image.tag=latest for local deployments (chart has no appVersion)
  • Added proper Auth0 secret creation commands with prefixed naming
  • Used heredoc for clean YAML file creation in OCI installation

Configuration Improvements

  • Added missing environment variables from service README to values.yaml:
    • AUTH0_TENANT (required)
    • HEIMDALL_KEY_ID (optional)
    • HEIMDALL_JWKS_URL (optional, with default)
    • LFX_API_GW (optional, with default)
  • Configured Auth0 authentication via v1-sync-helper-auth0-credentials secret
  • Updated deployment template to reference Auth0 credentials from secret

Documentation Cleanup

  • Streamlined environment variables section to show only variables with chart defaults
  • Added reference to main service README for complete variable documentation
  • Added comprehensive "Required Secrets" section
  • Used prefixed secret names to avoid conflicts in shared lfx namespace

Testing

Users can now deploy the service with proper authentication for both:

  • LFX v2 APIs (via Heimdall JWT)
  • LFX v1 APIs (via Auth0 JWT)

The deployment instructions provide all necessary commands for secret creation and chart installation.

- Specify client ID directly instead of pulling from secret
- Change default privateKeyKey to 'signer.pem'
- Aligns with existing Heimdall secret structure in platform stack

🤖 Generated with [GitHub Copilot](https://github.com/features/copilot) (via Zed)

Signed-off-by: Eric Searcy <[email protected]>
- Add local and OCI image deployment sections to chart README
- Configure Auth0 authentication via v1-sync-helper-auth0-credentials secret
- Add all missing environment variables from service README
- Simplify environment variables documentation to show only defaults
- Reference main service README for complete variable list
- Use heredoc for values.yaml creation in OCI installation

🤖 Generated with [GitHub Copilot](https://github.com/features/copilot) (via Zed)

Signed-off-by: Eric Searcy <[email protected]>
- Note that secrets are only required if they don't already exist
- Explain that heimdall-signer-cert should exist from LFX platform deployment
- Provide fallback instructions for missing Heimdall secret

🤖 Generated with [GitHub Copilot](https://github.com/features/copilot) (via Zed)

Signed-off-by: Eric Searcy <[email protected]>
@emsearcy emsearcy marked this pull request as ready for review November 27, 2025 01:01
@emsearcy emsearcy requested a review from a team as a code owner November 27, 2025 01:01
Copilot AI review requested due to automatic review settings November 27, 2025 01:01
Copilot finished reviewing on behalf of emsearcy November 27, 2025 01:04
Copy link

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 enhances the Helm chart deployment documentation and configuration for the lfx-v1-sync-helper service by adding comprehensive installation guides, properly configuring Auth0 authentication alongside existing Heimdall JWT signing, and documenting all required secrets and environment variables. The changes enable users to deploy the service with complete authentication support for both LFX v2 APIs (via Heimdall) and LFX v1 APIs (via Auth0).

Key Changes

  • Added Auth0 authentication configuration with dedicated secret references for v1 API calls
  • Refactored Heimdall configuration to use a static clientId value instead of reading from a secret
  • Expanded installation documentation with separate sections for local chart and OCI registry deployments, including all required secret creation commands

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File Description
charts/lfx-v1-sync-helper/values.yaml Added Auth0 secret configuration and new environment variables (AUTH0_TENANT, HEIMDALL_KEY_ID, HEIMDALL_JWKS_URL, LFX_API_GW); refactored Heimdall configuration to use clientId value
charts/lfx-v1-sync-helper/templates/deployment.yaml Updated to inject Auth0 credentials from secret; changed HEIMDALL_CLIENT_ID from secret reference to direct value
charts/lfx-v1-sync-helper/README.md Added comprehensive installation sections with secret creation commands; documented required secrets and environment variables in structured tables

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

| `COMMITTEE_SERVICE_URL` | `http://lfx-v2-committee-service.lfx.svc.cluster.local:8080` | Committee Service API URL |
| `HEIMDALL_JWKS_URL` | `http://lfx-platform-heimdall.lfx.svc.cluster.local:4457/.well-known/jwks` | JWKS endpoint URL |
| `LFX_API_GW` | `https://api-gw.dev.platform.linuxfoundation.org/` | LFX API Gateway URL |
| `LOG_LEVEL` | `info` | Log level |
Copy link

Copilot AI Nov 27, 2025

Choose a reason for hiding this comment

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

The LOG_LEVEL environment variable mentioned in the documentation table does not exist in the application's configuration. Based on the config.go file (lines 39, 63-64), the application only uses a boolean Debug flag controlled by the DEBUG environment variable, not a LOG_LEVEL variable.

Either remove this entry from the table or verify that this feature exists in the application code.

Suggested change
| `LOG_LEVEL` | `info` | Log level |

Copilot uses AI. Check for mistakes.
Comment on lines +99 to +114
### Environment Variables

The following environment variables have defaults configured in the chart's `app.environment` section:

| Variable | Default | Description |
|----------|---------|-------------|
| `NATS_URL` | `nats://lfx-platform-nats.lfx.svc.cluster.local:4222` | NATS server URL |
| `PROJECT_SERVICE_URL` | `http://lfx-v2-project-service.lfx.svc.cluster.local:8080` | Project Service API URL |
| `COMMITTEE_SERVICE_URL` | `http://lfx-v2-committee-service.lfx.svc.cluster.local:8080` | Committee Service API URL |
| `HEIMDALL_JWKS_URL` | `http://lfx-platform-heimdall.lfx.svc.cluster.local:4457/.well-known/jwks` | JWKS endpoint URL |
| `LFX_API_GW` | `https://api-gw.dev.platform.linuxfoundation.org/` | LFX API Gateway URL |
| `LOG_LEVEL` | `info` | Log level |
| `DEBUG` | `false` | Enable debug logging |
| `PORT` | `8080` | HTTP server port |
| `BIND` | `*` | Interface to bind on |

Copy link

Copilot AI Nov 27, 2025

Choose a reason for hiding this comment

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

[nitpick] The statement "The following environment variables have defaults configured in the chart's app.environment section" is misleading because some variables listed in the table have empty defaults (which are not truly defaults), and some configured variables are missing from the table.

Consider revising the text to clarify:

  • Which variables are required vs optional
  • That some variables like HEIMDALL_CLIENT_ID are set via the heimdall.clientId value (not app.environment)
  • That AUTH0_TENANT is required and has no default, despite being in app.environment

Alternatively, consider restructuring to have two separate tables: one for required variables and one for optional variables with defaults.

Copilot uses AI. Check for mistakes.
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