Skip to content

depend in nebari app library chart - #38

Open
pmeier wants to merge 14 commits into
mainfrom
nebari-app-library-chart
Open

depend in nebari app library chart#38
pmeier wants to merge 14 commits into
mainfrom
nebari-app-library-chart

Conversation

@pmeier

@pmeier pmeier commented Aug 28, 2026

Copy link
Copy Markdown
Member

Reference Issues or PRs

Fixes #13.

What does this implement/fix?

Put a x in the boxes that apply

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds a feature)
  • Breaking change (fix or feature that would cause existing features not to work as expected)
  • Documentation Update
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes, no API changes)
  • Build related changes
  • Other (please describe):

Testing

  • Did you test the pull request locally?
  • Did you add new tests?

Documentation

Access-centered content checklist

Text styling

  • The content is written with plain language (where relevant).
  • If there are headers, they use the proper header tags (with only one level-one header: H1 or # in markdown).
  • All links describe where they link to (for example, check the Nebari website).
  • This content adheres to the Nebari style guides.

Non-text content

  • All content is represented as text (for example, images need alt text, and videos need captions or descriptive transcripts).
  • If there are emojis, there are not more than three in a row.
  • Don't use flashing GIFs or videos.
  • If the content were to be read as plain text, it still makes sense, and no information is missing.

Any other comments?

@pmeier

pmeier commented Aug 28, 2026

Copy link
Copy Markdown
Member Author

Rendered chart comparison

Generated using these scripts

auth

nebariapp:
  # only for main
  enabled: true
  # only for main
  hostname: "serve.example.com"
  # only for main
  auth:
    enabled: true
    enforceAtGateway: false
  serve:
    enabled: true
    # only for PR
    hostname: "serve.example.com"
    # only for PR
    auth:
      enabled: true
      enforceAtGateway: true
  dashboard:
    enabled: true
    hostname: "dashboard.example.com"
    # only for PR
    auth:
      enabled: true
      enforceAtGateway: false
Diff

*** /tmp/sorted1.yaml	2026-09-03 15:42:53.332512037 +0200
--- /tmp/sorted2.yaml	2026-09-03 15:42:53.357512440 +0200
***************
*** 620,640 ****
      app.kubernetes.io/managed-by: Helm
      app.kubernetes.io/name: nebari-rayserve-pack
      app.kubernetes.io/version: 2.43.0
      helm.sh/chart: nebari-rayserve-pack-0.4.1
    name: release-name-nebari-rayserve-pack
  spec:
    auth:
      enabled: true
!     provider: keycloak
!     provisionClient: true
!     redirectURI: /oauth2/callback
!     scopes:
!       - openid
!       - profile
!       - email
!   gateway: public
    hostname: serve.example.com
    routing:
      routes:
        - pathPrefix: /
          pathType: PathPrefix
--- 620,634 ----
      app.kubernetes.io/managed-by: Helm
      app.kubernetes.io/name: nebari-rayserve-pack
      app.kubernetes.io/version: 2.43.0
      helm.sh/chart: nebari-rayserve-pack-0.4.1
    name: release-name-nebari-rayserve-pack
+   namespace: default
  spec:
    auth:
      enabled: true
!     enforceAtGateway: true
    hostname: serve.example.com
    routing:
      routes:
        - pathPrefix: /
          pathType: PathPrefix
***************
*** 652,672 ****
      app.kubernetes.io/managed-by: Helm
      app.kubernetes.io/name: nebari-rayserve-pack
      app.kubernetes.io/version: 2.43.0
      helm.sh/chart: nebari-rayserve-pack-0.4.1
    name: release-name-nebari-rayserve-pack-dashboard
  spec:
    auth:
      enabled: true
!     provider: keycloak
!     provisionClient: true
!     redirectURI: /oauth2/callback
!     scopes:
!       - openid
!       - profile
!       - email
!   gateway: public
    hostname: dashboard.example.com
    landingPage:
      category: Data Science
      description: Monitor and manage Ray clusters and Serve deployments
      displayName: Ray Dashboard
--- 646,660 ----
      app.kubernetes.io/managed-by: Helm
      app.kubernetes.io/name: nebari-rayserve-pack
      app.kubernetes.io/version: 2.43.0
      helm.sh/chart: nebari-rayserve-pack-0.4.1
    name: release-name-nebari-rayserve-pack-dashboard
+   namespace: default
  spec:
    auth:
      enabled: true
!     enforceAtGateway: false
    hostname: dashboard.example.com
    landingPage:
      category: Data Science
      description: Monitor and manage Ray clusters and Serve deployments
      displayName: Ray Dashboard

both

nebariapp:
  # only for main
  enabled: true
  # only for main
  hostname: "serve.example.com"
  serve:
    enabled: true
    # only for PR
    hostname: "serve.example.com"
  dashboard:
    enabled: true
    hostname: "dashboard.example.com"
Diff

*** /tmp/sorted1.yaml	2026-09-03 15:42:53.385512893 +0200
--- /tmp/sorted2.yaml	2026-09-03 15:42:53.412513329 +0200
***************
*** 620,631 ****
      app.kubernetes.io/managed-by: Helm
      app.kubernetes.io/name: nebari-rayserve-pack
      app.kubernetes.io/version: 2.43.0
      helm.sh/chart: nebari-rayserve-pack-0.4.1
    name: release-name-nebari-rayserve-pack
  spec:
-   gateway: public
    hostname: serve.example.com
    routing:
      routes:
        - pathPrefix: /
          pathType: PathPrefix
--- 620,631 ----
      app.kubernetes.io/managed-by: Helm
      app.kubernetes.io/name: nebari-rayserve-pack
      app.kubernetes.io/version: 2.43.0
      helm.sh/chart: nebari-rayserve-pack-0.4.1
    name: release-name-nebari-rayserve-pack
+   namespace: default
  spec:
    hostname: serve.example.com
    routing:
      routes:
        - pathPrefix: /
          pathType: PathPrefix
***************
*** 643,654 ****
      app.kubernetes.io/managed-by: Helm
      app.kubernetes.io/name: nebari-rayserve-pack
      app.kubernetes.io/version: 2.43.0
      helm.sh/chart: nebari-rayserve-pack-0.4.1
    name: release-name-nebari-rayserve-pack-dashboard
  spec:
-   gateway: public
    hostname: dashboard.example.com
    landingPage:
      category: Data Science
      description: Monitor and manage Ray clusters and Serve deployments
      displayName: Ray Dashboard
--- 643,654 ----
      app.kubernetes.io/managed-by: Helm
      app.kubernetes.io/name: nebari-rayserve-pack
      app.kubernetes.io/version: 2.43.0
      helm.sh/chart: nebari-rayserve-pack-0.4.1
    name: release-name-nebari-rayserve-pack-dashboard
+   namespace: default
  spec:
    hostname: dashboard.example.com
    landingPage:
      category: Data Science
      description: Monitor and manage Ray clusters and Serve deployments
      displayName: Ray Dashboard

dashboard

nebariapp:
  # only for main
  enabled: true
  serve:
    enabled: false
  dashboard:
    enabled: true
    hostname: "dashboard.example.com"
Diff

*** /tmp/sorted1.yaml	2026-09-03 15:42:53.176509517 +0200
--- /tmp/sorted2.yaml	2026-09-03 15:42:53.201509921 +0200
***************
*** 620,631 ****
      app.kubernetes.io/managed-by: Helm
      app.kubernetes.io/name: nebari-rayserve-pack
      app.kubernetes.io/version: 2.43.0
      helm.sh/chart: nebari-rayserve-pack-0.4.1
    name: release-name-nebari-rayserve-pack-dashboard
  spec:
-   gateway: public
    hostname: dashboard.example.com
    landingPage:
      category: Data Science
      description: Monitor and manage Ray clusters and Serve deployments
      displayName: Ray Dashboard
--- 620,631 ----
      app.kubernetes.io/managed-by: Helm
      app.kubernetes.io/name: nebari-rayserve-pack
      app.kubernetes.io/version: 2.43.0
      helm.sh/chart: nebari-rayserve-pack-0.4.1
    name: release-name-nebari-rayserve-pack-dashboard
+   namespace: default
  spec:
    hostname: dashboard.example.com
    landingPage:
      category: Data Science
      description: Monitor and manage Ray clusters and Serve deployments
      displayName: Ray Dashboard

real-world

nebariapp:
  enabled: true
  # Keep the Serve endpoint internal-only (per upstream README's
  # recommendation). Notebooks access it via cluster DNS at
  # rayserve-pack-nebari-rayserve-serve-svc.ray.svc.cluster.local:8000.
  serve:
    enabled: false
  dashboard:
    enabled: true
    hostname: ray-dashboard.atep-dev.openteams.local
    landingPage:
      enabled: true
    # only for PR
    auth:
      enabled: true
      provider: keycloak
      provisionClient: true
      redirectURI: /oauth2/callback
  # only for main
  auth:
    enabled: true
    provider: keycloak
    provisionClient: true
    redirectURI: /oauth2/callback
Diff

*** /tmp/sorted1.yaml	2026-09-03 15:42:53.228510357 +0200
--- /tmp/sorted2.yaml	2026-09-03 15:42:53.252510745 +0200
***************
*** 620,640 ****
      app.kubernetes.io/managed-by: Helm
      app.kubernetes.io/name: nebari-rayserve-pack
      app.kubernetes.io/version: 2.43.0
      helm.sh/chart: nebari-rayserve-pack-0.4.1
    name: release-name-nebari-rayserve-pack-dashboard
  spec:
    auth:
      enabled: true
      provider: keycloak
      provisionClient: true
      redirectURI: /oauth2/callback
-     scopes:
-       - openid
-       - profile
-       - email
-   gateway: public
    hostname: ray-dashboard.atep-dev.openteams.local
    landingPage:
      category: Data Science
      description: Monitor and manage Ray clusters and Serve deployments
      displayName: Ray Dashboard
--- 620,636 ----
      app.kubernetes.io/managed-by: Helm
      app.kubernetes.io/name: nebari-rayserve-pack
      app.kubernetes.io/version: 2.43.0
      helm.sh/chart: nebari-rayserve-pack-0.4.1
    name: release-name-nebari-rayserve-pack-dashboard
+   namespace: default
  spec:
    auth:
      enabled: true
      provider: keycloak
      provisionClient: true
      redirectURI: /oauth2/callback
    hostname: ray-dashboard.atep-dev.openteams.local
    landingPage:
      category: Data Science
      description: Monitor and manage Ray clusters and Serve deployments
      displayName: Ray Dashboard

serve

nebariapp:
  # only for main
  enabled: true
  # only for main
  hostname: "serve.example.com"
  serve:
    enabled: true
    # only for PR
    hostname: "serve.example.com"
  dashboard:
    enabled: false
Diff

*** /tmp/sorted1.yaml	2026-09-03 15:42:53.280511197 +0200
--- /tmp/sorted2.yaml	2026-09-03 15:42:53.303511568 +0200
***************
*** 620,631 ****
      app.kubernetes.io/managed-by: Helm
      app.kubernetes.io/name: nebari-rayserve-pack
      app.kubernetes.io/version: 2.43.0
      helm.sh/chart: nebari-rayserve-pack-0.4.1
    name: release-name-nebari-rayserve-pack
  spec:
-   gateway: public
    hostname: serve.example.com
    routing:
      routes:
        - pathPrefix: /
          pathType: PathPrefix
--- 620,631 ----
      app.kubernetes.io/managed-by: Helm
      app.kubernetes.io/name: nebari-rayserve-pack
      app.kubernetes.io/version: 2.43.0
      helm.sh/chart: nebari-rayserve-pack-0.4.1
    name: release-name-nebari-rayserve-pack
+   namespace: default
  spec:
    hostname: serve.example.com
    routing:
      routes:
        - pathPrefix: /
          pathType: PathPrefix

Comment thread chart/values.yaml
Comment thread chart/values.yaml
Comment thread chart/values.yaml
Comment thread chart/values.yaml
Comment thread chart/values.yaml
Comment thread chart/values.yaml
Comment thread chart/templates/nebariapp.yaml
Comment thread chart/templates/NOTES.txt
Comment thread .github/workflows/lint.yaml
Comment thread .github/workflows/test.yaml
Comment thread .gitignore
Comment thread chart/Chart.yaml Outdated
@pmeier
pmeier marked this pull request as ready for review September 3, 2026 13:44

@oren-openteams oren-openteams left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Review

Nice refactor — the library-chart approach cleanly solves #13 by making rayserve-pack a thin consumer that can't drop fields it doesn't know about. The 91-line reduction in chart/templates/nebariapp.yaml is a real win, per-endpoint auth (serve.auth.* / dashboard.auth.*) is architecturally right, and requiring hostname when the component is enabled fixes a real silent-no-op bug. Committing chart/Chart.lock is the right pattern for reproducible builds.

Three small asks before merge (independent of the operator#184 blocker already noted on Chart.yaml):

1. Chart.lock version mismatch

chart/Chart.yaml requires nebari-app version 0.1.3, but chart/Chart.lock pins 0.1.2. helm dep update and re-commit the lock so CI actually pulls what the chart declares.

2. Add one guard test that closes the #13 loop

Removing the shallow "does templating work" lint tests is defensible — you're right that they don't check output. But the whole point of #13 is that previously-dropped fields (enforceAtGateway, forwardAccessToken, denyRedirect, clientSecretRef, groups, issuerURL, spaClient, deviceFlowClient, keycloakConfig, tokenExchange) now propagate end-to-end. The passthrough lives in the library chart now, but a future library-chart regression could silently reintroduce field-dropping without any test in this repo catching it.

Suggest one CI step (either in lint.yaml or as a new render-assertion test) that sets a couple of previously-dropped values and asserts they show up in the rendered NebariApp CR. Something like:

- name: Assert advanced auth fields propagate through to NebariApp
  run: |
    helm template test chart/ \
      --set nebariapp.serve.enabled=true \
      --set nebariapp.serve.hostname=test.example.com \
      --set nebariapp.serve.auth.enabled=true \
      --set nebariapp.serve.auth.enforceAtGateway=false \
      --set nebariapp.serve.auth.forwardAccessToken=true \
      > /tmp/rendered.yaml

    yq 'select(.kind == "NebariApp" and .metadata.name == "test-nebari-rayserve") \
        | .spec.auth.enforceAtGateway' /tmp/rendered.yaml \
      | grep -qx 'false'
    yq 'select(.kind == "NebariApp" and .metadata.name == "test-nebari-rayserve") \
        | .spec.auth.forwardAccessToken' /tmp/rendered.yaml \
      | grep -qx 'true'

Same shape for one field on dashboard.auth too, so both NebariApps are covered. That's the smallest test that would have failed on main prior to this PR, and would fail again if the passthrough ever regressed.

3. Address the "reinstate" self-flag on NOTES.txt line 6

You noted "Flag for myself: reinstate." — worth resolving before merge so the review has visibility into what's coming back.

Independent — worth double-checking

The Chart.yaml comment says this is blocked by nebari-operator#184. Worth eyeballing that whether the operator PR actually moves the library chart under a nebari-dev-owned registry (vs. addressing something else) — if the registry-ownership concern isn't resolved there, it should be tracked as a separate blocker on this PR.

Otherwise LGTM once the three items above land.

@pmeier

pmeier commented Sep 7, 2026

Copy link
Copy Markdown
Member Author

@oren-openteams the missing rendered chart correctness test (issue 2. in #38 (review)) is a pre-existing issue. I've opened #39 to track it, but wouldn't include it into this PR unless you insist.

@oren-openteams oren-openteams left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks for the refactor — the ~90 → ~20 line template drop and the per-endpoint auth model both feel like real wins. A few things I'd like resolved before this lands:

Guard against silent value-schema regressions

The old top-level keys (nebariapp.enabled, nebariapp.hostname, nebariapp.auth, nebariapp.gateway, nebariapp.service.servePort/dashboardPort) are quietly dropped. Users with existing values.yaml overrides who bump the chart without migrating will silently render with no NebariApp CRs — both serve.enabled and dashboard.enabled default to false now.

Consider a fail-guard for the removed top-level keys so this becomes a loud install-time error rather than a silent no-op, e.g.:

{{- if hasKey .Values.nebariapp "enabled" -}}
{{- fail "nebariapp.enabled was removed in 0.4.0; set nebariapp.serve.enabled and/or nebariapp.dashboard.enabled instead." -}}
{{- end -}}

Same idea for nebariapp.hostname (moved under the endpoint), nebariapp.auth, nebariapp.gateway. Catches CI pipelines that don't read release notes.

Clarifications

  • gateway disposition. I don't see where the top-level nebariapp.gateway (was public) went. Is it now per-endpoint (serve.gateway / dashboard.gateway), library-defaulted, or removed entirely? Worth calling out.
  • auth.scopes default. The old values.yaml had auth.scopes: [openid, profile, email]. I don't see the equivalent in the new schema. If the library chart supplies it, worth documenting. If not, existing configs relying on the default will silently drop those scopes.
  • Library chart pin at 0.1.1. Any stability commitment on nebari-app at 0.1.x? A minor bump there could break every downstream nebariapp-consuming chart at once — worth pinning to a range or documenting the compat contract.

Nits

  • Typo in the two fail templates: nebariapps.serve.hostname is required... → should be nebariapp. (no plural). Same for the dashboard fail.
  • Cosmetic: double space in NOTES.txt (NebariApp for {{ $name }} → single space).

Otherwise

Once the above are addressed, this looks great. The library-chart split is a good direction and per-endpoint auth is the right shape. Happy to re-review.

@pmeier

pmeier commented Sep 10, 2026

Copy link
Copy Markdown
Member Author

Consider a fail-guard for the removed top-level keys so this becomes a loud install-time error rather than a silent no-op. [...] Catches CI pipelines that don't read release notes.

I've put the guards in, but I personally don't feel they are necessary. If anyone in all seriousness let's CI pipelines upgrade software without anyone looking at release notes, they likely deserve it.

Library chart pin at 0.1.1. Any stability commitment on nebari-app at 0.1.x? A minor bump there could break every downstream nebariapp-consuming chart at once — worth pinning to a range or documenting the compat contract.

Since we have a lock file, installing this pack will always pull 0.1.1 and thus is not affected by any upstream updates. This would only trickle down if we update our dependencies. And in that case same sentiment as above: if we run the update and do not care about the release notes, we deserve what is coming for us.

gateway disposition. I don't see where the top-level nebariapp.gateway (was public) went. Is it now per-endpoint (serve.gateway / dashboard.gateway), library-defaulted, or removed entirely? Worth calling out.

auth.scopes default. The old values.yaml had auth.scopes: [openid, profile, email]. I don't see the equivalent in the new schema. If the library chart supplies it, worth documenting. If not, existing configs relying on the default will silently drop those scopes.

Both of them are removed from the values as they are the CR defaults. See

If the user wants to set these, it now has to happen on component level, e.g. nebariapp.serve.auth and nebariapp.dashboard.auth. This needs to be called out in the release notes, but is also part of the upgrade guards you requested.

@pmeier

pmeier commented Sep 10, 2026

Copy link
Copy Markdown
Member Author

Breaking changes that need to make their way into the release notes:

  • The global nebariapp.enabled was removed. Set nebariapp.serve.enabled and nebariapp.dashboard.enabled individually.
  • nebariapp.dashboard.enabled default value changed from true to false. Previously, this was shadowed by the default nebariapp.enabled=false so the dashboard NebariApp was not rendered by default. Since the global switch is now gone, the default change was necessary to avoid rendering the dashboard NebariApp by default.
  • nebariapp.service was removed. Set the old nebariapp.service.servePort as nebariapp.serve.service.port and nebariapp.service.dashboardPort as nebariapp.serve.dashboard.port. Set the old nebariapp.service.name as nebariapp.serve.service.name and nebariapp.dashboard.service.name individually.
  • nebariapp.auth was removed. Set nebariapp.serve.auth and nebariapp.dashboard.auth individually.
  • nebariapp.gateway was removed. Set nebariapp.serve.gateway and nebariapp.dashboard.gateway individually.

Comment thread chart/Chart.yaml
description: A Nebari Software Pack for Ray Serve
type: application
version: 0.4.1
version: 0.5.0

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I've bumped the chart version as part of this PR. LMK if you want to do it in a follow-up instead.

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.

NebariApp template drops most AuthConfig fields, duplicates auth block

3 participants