Skip to content

feat: add alpha-config helpers and deprecation guards#405

Open
pierluigilenoci wants to merge 3 commits intooauth2-proxy:mainfrom
pierluigilenoci:refactor/alpha-config-helpers
Open

feat: add alpha-config helpers and deprecation guards#405
pierluigilenoci wants to merge 3 commits intooauth2-proxy:mainfrom
pierluigilenoci:refactor/alpha-config-helpers

Conversation

@pierluigilenoci
Copy link
Copy Markdown
Member

Summary

Structural improvements to alpha config handling, cherry-picked from the non-bugfix parts of #402:

  • Add alpha-config.source and alpha-config.name template helpers to centralize scattered inline conditionals for alpha config type detection and name resolution
  • Add deprecation guards that fail fast on invalid alpha config combinations:
    • existingConfig + existingSecret set together
    • External alpha source combined with generated content (configData, configFile, etc.)
  • Simplify secret-alpha.yaml and deployment.yaml volume definitions using the new helpers
  • Only compute checksum/alpha-config annotation for generated alpha configs (not external)
  • Document mutual exclusivity of existingConfig/existingSecret in values.yaml
  • Add CI test for alphaConfig.existingSecret scenario

Not included (intentionally):

Test plan

  • helm lint passes for all 24 CI value files
  • helm template with alphaconfig-7-existing-secret-values.yaml mounts external secret correctly (no generated secret)
  • helm template with alphaconfig-1-legacy-true-values.yaml still generates alpha secret with checksum
  • Deprecation guards reject invalid combinations (existingConfig + existingSecret, external + generated)

- Add alpha-config.source and alpha-config.name template helpers
  to centralize alpha config type detection and name resolution
- Add deprecation guards: fail fast when existingConfig and
  existingSecret are both set, or when external sources are
  combined with generated alpha content
- Simplify secret-alpha.yaml and deployment.yaml volume definitions
  using the new helpers
- Only compute checksum/alpha-config for generated alpha configs
- Document mutual exclusivity of existingConfig/existingSecret
- Add CI test for alphaConfig.existingSecret scenario

Signed-off-by: Pierluigi Lenoci <pierluigi.lenoci@gmail.com>
Copy link
Copy Markdown

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 improves the Helm chart’s alpha config handling by centralizing alpha config source/name resolution into reusable helpers, simplifying templates that mount/generate alpha config, and adding guardrails against invalid value combinations.

Changes:

  • Add oauth2-proxy.alpha-config.source and oauth2-proxy.alpha-config.name helpers to standardize alpha config selection and naming.
  • Add fail-fast guards for invalid alpha config combinations (external sources vs generated content; existingConfig vs existingSecret).
  • Update deployment/secret templates and add a CT install values file to cover the alphaConfig.existingSecret scenario.

Reviewed changes

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

Show a summary per file
File Description
helm/oauth2-proxy/values.yaml Documents mutual exclusivity for alphaConfig.existingConfig and alphaConfig.existingSecret.
helm/oauth2-proxy/templates/_helpers.tpl Introduces centralized alpha-config source + name helpers.
helm/oauth2-proxy/templates/secret-alpha.yaml Generates alpha Secret only when the resolved source is generated, and uses helper for naming.
helm/oauth2-proxy/templates/deployment.yaml Uses resolved alpha source to control checksum annotation and volume mounts/names.
helm/oauth2-proxy/templates/deprecation.yaml Adds guardrails to reject invalid alpha config combinations (currently gated by checkDeprecation).
helm/oauth2-proxy/ci/alphaconfig-7-existing-secret-values.yaml Adds CI install scenario for alphaConfig.existingSecret.
helm/oauth2-proxy/Chart.yaml Bumps chart version and updates Artifact Hub change notes.

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

Use fully qualified field names (alphaConfig.serverConfigData, etc.)
in the error message for clarity.

Signed-off-by: Pierluigi Lenoci <pierluigi.lenoci@gmail.com>
Copy link
Copy Markdown

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

Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.


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

Copy link
Copy Markdown

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

Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.


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

fullname is already 63 chars max; appending '-alpha' could exceed
the Kubernetes name limit.

Signed-off-by: Pierluigi Lenoci <pierluigi.lenoci@gmail.com>
Copy link
Copy Markdown

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

Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.


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

@pierluigilenoci
Copy link
Copy Markdown
Member Author

Hi @tuunit, @jkroepke -- friendly ping on this PR. CI is green and it's been waiting for review for a while. Would you be able to take a look when you get a chance? Happy to address any feedback. Thank you!

@jkroepke
Copy link
Copy Markdown
Contributor

@pierluigilenoci I would like to kindly ask you why you ping me? I have no maintainer or merge power here.

@pierluigilenoci
Copy link
Copy Markdown
Member Author

@jkroepke Apologies for the unnecessary ping — I mistakenly assumed maintainer status from your activity in the org. Sorry for the noise! @tuunit could you take a look when you get a chance? Thank you.

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.

3 participants