Move to shared service chart by AlanSherman · Pull Request #70 · linuxfoundation/lfx-v2-meeting-service

AlanSherman · 2025-10-09T16:35:40Z

No description provided.

Signed-off-by: Alan Sherman <asherman@linuxfoundation.org>

coderabbitai · 2025-10-09T16:35:48Z

Walkthrough

Renames the Helm chart to lfx-one-meeting-service, bumps version to 2.0.0, and introduces a dependency on lfx-service. Removes local templates for Deployment, Service, ServiceAccount, HTTPRoute, Traefik middleware, RuleSet, and NATS KV buckets. Replaces values.yaml with a consolidated lfx-service configuration for image, deployment, routing, auth rules, Heimdall, and KV buckets.

Changes

Cohort / File(s)	Summary
Chart metadata and dependency `charts/lfx-v2-meeting-service/Chart.yaml`	Renamed chart, updated description, version 0.4.16 -> 2.0.0, added dependency on lfx-service 1.0.0 (file repo, OCI commented).
Core workload templates removed `charts/lfx-v2-meeting-service/templates/deployment.yaml`, `.../templates/service.yaml`, `.../templates/serviceaccount.yaml`	Deleted Deployment, Service, and conditional ServiceAccount manifests.
Routing and middleware templates removed `.../templates/httproute.yaml`, `.../templates/heimdall-middleware.yaml`	Deleted Gateway HTTPRoute and Traefik Heimdall middleware templates, including conditional render logic.
Authorization rules removed `.../templates/ruleset.yaml`	Deleted comprehensive RuleSet covering OpenAPI, webhooks, meetings, registrants, past meetings, participants, and summaries (incl. OpenFGA conditionals).
NATS KV bucket templates removed `.../templates/nats-kv-buckets.yaml`	Deleted conditional JetStream KeyValue bucket manifests for multiple buckets.
Values consolidation to lfx-service `charts/lfx-v2-meeting-service/values.yaml`	Replaced legacy values with lfx-service schema: global, image (repo/tag updated), deployment.environment migrated, httpRoute, heimdall, ruleSet (IDs and routes), and nats.kvBuckets list. Removed previous image/app/traefik/lfx/service/serviceAccount/openfga and per-bucket configs.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  participant U as User/CI
  participant H as Helm
  participant P as Parent Chart (lfx-one-meeting-service)
  participant D as Dependency (lfx-service)

  U->>H: helm install/upgrade
  H->>P: Render with values.yaml
  P->>D: Pass lfx-service.* values
  Note over P,D: Parent no longer renders Deployment/Service/Rules<br/>Delegates to dependency chart
  D->>D: Generate Deployment, Service, HTTPRoute, Middleware, RuleSet, KV buckets
  D-->>H: Rendered manifests
  H-->>U: Apply manifests to cluster

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

[LFXV2-26] Initial meeting service implementation #1 — Introduces and restructures the Helm chart/templates later removed here; directly related to this refactor and consolidation.
add heimdall middleware #29 — Adds conditional Heimdall middleware template that this PR removes and replaces via dependency-driven config.
update chart to use value/valueFrom for env #17 — Changes env var rendering logic that this PR supersedes by moving environment into lfx-service.deployment.environment.

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Description Check	⚠️ Warning	The pull request lacks any description content, so there is no information provided to explain the changes or their intent in the context of the diff.	Please provide a brief description summarizing the changes and objectives of this pull request to aid reviewers.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Title Check	✅ Passed	The title succinctly describes migrating the chart to a shared service chart, which accurately reflects the main change of consolidating the meeting service into a common lfx-service chart.
Docstring Coverage	✅ Passed	No functions found in the changes. Docstring coverage check skipped.

✨ Finishing touches

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment
Commit unit tests in branch asherman/helm-service

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

Copilot

Pull Request Overview

This PR migrates the lfx-v2-meeting-service chart to use a shared service chart architecture. The service is transitioning from standalone Helm templates to using a shared base chart pattern for consistency and maintainability across LFX services.

Restructured values.yaml to use the new lfx-service chart structure
Updated Chart.yaml to depend on the shared lfx-service chart (version 1.0.0)
Removed individual template files that are now provided by the base chart

Reviewed Changes

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

File	Description
charts/lfx-v2-meeting-service/values.yaml	Complete restructure to use lfx-service chart format with nested configuration
charts/lfx-v2-meeting-service/Chart.yaml	Added dependency on shared lfx-service chart and updated naming
Template files (deployment.yaml, service.yaml, etc.)	Removed individual templates now provided by base chart

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

Copilot · 2025-10-09T16:36:25Z

charts/lfx-v2-meeting-service/values.yaml

+      ZOOM_WEBHOOK_SECRET_TOKEN:
+        value: ""
+      LFX_ENVIRONMENT:
+        value: ""


The LFX_ENVIRONMENT variable is set to an empty string instead of a default value like 'prod'. This could cause issues if the application expects a valid environment value and doesn't handle empty strings appropriately.

Suggested change

value: ""

value: "prod"

Copilot · 2025-10-09T16:36:26Z

charts/lfx-v2-meeting-service/values.yaml

+          methods: ["GET"]
+          routes: ["/meetings/:id/settings"]
+        authorization:
+          relation: organizer


The authorization rule for getting meeting settings requires 'organizer' relation, but the original ruleset used 'auditor' relation. This is a more restrictive change that may break existing functionality for users who should be able to view settings but aren't organizers.

Suggested change

relation: organizer

relation: auditor

Copilot · 2025-10-09T16:36:26Z

charts/lfx-v2-meeting-service/values.yaml

+          methods: ["GET"]
+          routes: ["/meetings/:id/registrants"]
+        authorization:
+          relation: organizer


The authorization rule for listing meeting registrants now requires 'organizer' relation, but the original ruleset used 'auditor' relation. This changes the access control model and may prevent legitimate users from viewing registrant lists.

Suggested change

relation: organizer

relation: auditor

coderabbitai

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Disabled knowledge base sources:

Jira integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 850d5dc and 65671b9.

📒 Files selected for processing (9)

charts/lfx-v2-meeting-service/Chart.yaml (1 hunks)
charts/lfx-v2-meeting-service/templates/deployment.yaml (0 hunks)
charts/lfx-v2-meeting-service/templates/heimdall-middleware.yaml (0 hunks)
charts/lfx-v2-meeting-service/templates/httproute.yaml (0 hunks)
charts/lfx-v2-meeting-service/templates/nats-kv-buckets.yaml (0 hunks)
charts/lfx-v2-meeting-service/templates/ruleset.yaml (0 hunks)
charts/lfx-v2-meeting-service/templates/service.yaml (0 hunks)
charts/lfx-v2-meeting-service/templates/serviceaccount.yaml (0 hunks)
charts/lfx-v2-meeting-service/values.yaml (1 hunks)

💤 Files with no reviewable changes (7)

charts/lfx-v2-meeting-service/templates/heimdall-middleware.yaml
charts/lfx-v2-meeting-service/templates/nats-kv-buckets.yaml
charts/lfx-v2-meeting-service/templates/serviceaccount.yaml
charts/lfx-v2-meeting-service/templates/deployment.yaml
charts/lfx-v2-meeting-service/templates/httproute.yaml
charts/lfx-v2-meeting-service/templates/ruleset.yaml
charts/lfx-v2-meeting-service/templates/service.yaml

coderabbitai · 2025-10-09T16:40:33Z

charts/lfx-v2-meeting-service/Chart.yaml

+  - name: lfx-service
+    version: 1.0.0
+    repository: file://../../../lfx-v2-helm/charts/lfx-service
+    # repository: oci://ghcr.io/linuxfoundation/lfx-service/chart


⚠️ Potential issue | 🔴 Critical

Fix the dependency repository path — it currently escapes the repo.

file://../../../lfx-v2-helm/charts/lfx-service climbs three levels up from charts/lfx-v2-meeting-service, so Helm will look for the dependency outside this repository and helm dependency build will fail in CI/packaging. Point the file URL at the actual in-repo location (two .. segments from here) or switch to the published OCI repo.

- repository: file://../../../lfx-v2-helm/charts/lfx-service + repository: file://../../lfx-v2-helm/charts/lfx-service

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

- name: lfx-service

version: 1.0.0

repository: file://../../../lfx-v2-helm/charts/lfx-service

# repository: oci://ghcr.io/linuxfoundation/lfx-service/chart

- name: lfx-service

version: 1.0.0

repository: file://../../lfx-v2-helm/charts/lfx-service

# repository: oci://ghcr.io/linuxfoundation/lfx-service/chart

🤖 Prompt for AI Agents

In charts/lfx-v2-meeting-service/Chart.yaml around lines 12 to 15, the dependency repository path climbs three levels (file://../../../lfx-v2-helm/charts/lfx-service) which escapes the repo and breaks helm dependency build in CI; update the repository to point to the in-repo location using two parent segments (file://../../lfx-v2-helm/charts/lfx-service) or replace it with the published OCI URL (oci://ghcr.io/linuxfoundation/lfx-service/chart) so Helm resolves the dependency within the repository.

Move to shared service chart

65671b9

Signed-off-by: Alan Sherman <asherman@linuxfoundation.org>

AlanSherman requested a review from a team as a code owner October 9, 2025 16:35

Copilot AI review requested due to automatic review settings October 9, 2025 16:35

Copilot AI reviewed Oct 9, 2025

View reviewed changes

coderabbitai bot reviewed Oct 9, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move to shared service chart#70

Move to shared service chart#70
AlanSherman wants to merge 1 commit intomainfrom
asherman/helm-service

AlanSherman commented Oct 9, 2025

Uh oh!

coderabbitai bot commented Oct 9, 2025 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Oct 9, 2025

Uh oh!

Copilot AI Oct 9, 2025

Uh oh!

Copilot AI Oct 9, 2025

Uh oh!

coderabbitai bot left a comment

Uh oh!

coderabbitai bot Oct 9, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

AlanSherman commented Oct 9, 2025

Uh oh!

coderabbitai bot commented Oct 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Sequence Diagram(s)

Estimated code review effort

Possibly related PRs

Pre-merge checks and finishing touches

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Oct 9, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 9, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 9, 2025

Choose a reason for hiding this comment

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

coderabbitai bot Oct 9, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

coderabbitai bot commented Oct 9, 2025 •

edited

Loading