Skip to content

⚡️(helm) create a dedicated svc and deployment for yprovider converter#2358

Merged
lunika merged 3 commits into
mainfrom
enhancement/y-provider-helm
May 28, 2026
Merged

⚡️(helm) create a dedicated svc and deployment for yprovider converter#2358
lunika merged 3 commits into
mainfrom
enhancement/y-provider-helm

Conversation

@lunika
Copy link
Copy Markdown
Member

@lunika lunika commented May 26, 2026

Purpose

We want to isolate the converter service built in the yprovider server in order to not mix conversion and collaboration together. We decided to create a dedicated service and deployment, by default they use the values defined in yProvider values but all can be overridden in yProvider.converter.
Also, by default these new service and deployment are not enabled, they must be explicitly enabled and configured. If not it works like now with only one service and deployment for the yProvider server.

Proposal

  • ⚡️(helm) create a dedicated svc and deployment for yprovider converter

@lunika lunika requested a review from AntoLC May 26, 2026 20:30
@lunika lunika self-assigned this May 26, 2026
@lunika lunika force-pushed the enhancement/y-provider-helm branch from 15debb0 to 14265f5 Compare May 26, 2026 20:31
@AntoLC
Copy link
Copy Markdown
Collaborator

AntoLC commented May 27, 2026

@CodeRabbit review

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 27, 2026

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@AntoLC AntoLC added the helm label May 27, 2026
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 27, 2026

Review Change Stack

Walkthrough

This PR adds infrastructure for a dedicated yProvider converter component to the Helm chart. It introduces a new values schema block (yProvider.converter) with full deployment and service configuration, adds a naming helper function, creates Kubernetes Deployment and Service templates with support for merged configuration, volume mounting, persistence, probes, scheduling constraints, and optional PodDisruptionBudget, and configures the dev environment to enable the converter with appropriate backend environment variables and routing.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately describes the main change: creating a dedicated service and deployment for the yProvider converter, which is the primary objective reflected across all modified files.
Description check ✅ Passed The description directly explains the purpose and implementation of the changeset: isolating the converter service by creating dedicated Kubernetes resources that inherit from yProvider but can be overridden.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch enhancement/y-provider-helm

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

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

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/helm/impress/templates/yprovider_deployment_converter.yaml`:
- Around line 8-23: The current use of default for map-typed fields causes child
converter maps to fully replace parent yProvider maps and drop keys; update the
template to merge maps instead of replacing them by using mergeOverwrite for
map-valued variables (so inheritance + partial override works). Specifically,
replace default(...) occurrences for dpAnnotations, podAnnotations,
securityContext, resources, nodeSelector, affinity, persistence, and
extraVolumeMounts with mergeOverwrite calls that merge $yProvider.* and
$converter.* (keep default for scalar/array fields like replicas, command, args,
sidecars where appropriate). Ensure you reference the template variables
$dpAnnotations, $podAnnotations, $securityContext, $resources, $nodeSelector,
$affinity, $persistence, and $extraVolumeMounts when making the changes.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 1d2eca03-86e3-45a6-9e02-130b47d890e4

📥 Commits

Reviewing files that changed from the base of the PR and between 3264e29 and 14265f5.

📒 Files selected for processing (6)
  • CHANGELOG.md
  • src/helm/env.d/dev/values.impress.yaml.gotmpl
  • src/helm/impress/templates/_helpers.tpl
  • src/helm/impress/templates/yprovider_deployment_converter.yaml
  • src/helm/impress/templates/yprovider_svc_converter.yaml
  • src/helm/impress/values.yaml

Comment thread src/helm/impress/templates/yprovider_deployment_converter.yaml Outdated
Copy link
Copy Markdown
Collaborator

@AntoLC AntoLC left a comment

Choose a reason for hiding this comment

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

Should we replicate this configuration in the docker environment as well, to stay more or less iso with the production, 1 container about collab and 1 about conversion ?

@lunika lunika force-pushed the enhancement/y-provider-helm branch from 14265f5 to 652a4d7 Compare May 27, 2026 09:25
@lunika lunika requested a review from AntoLC May 27, 2026 09:39
@lunika lunika force-pushed the enhancement/y-provider-helm branch from 652a4d7 to 29216ce Compare May 27, 2026 16:32
@lunika lunika added the preview label May 28, 2026
@github-actions
Copy link
Copy Markdown
Contributor

🚀 Preview will be available at https://2358-docs.ppr-docs.beta.numerique.gouv.fr/

You can use the existing account with these credentials:

  • username: docs
  • password: docs

You can also create a new account if you want to.

Once this Pull Request is merged, the preview will be destroyed.

lunika added 2 commits May 28, 2026 09:13
We want to isolate the converter service built in the yprovider server in
order to not mix conversion and collaboration together. We decided to
create a dedicated service and deployment, by default they use the
values defined in yProvider values but all can be overridden in
yProvider.converter.
Also, by default these new service and deployment are not enabled, they
must be explictly enabled and configured. If not it works like now with
only one service and deployment for the yProvider server.
We wanto to have in the compose stack a dedicated service for the
converter like we did in the helm chart. The change is also made with
the e2e compose file
@lunika lunika force-pushed the enhancement/y-provider-helm branch from 29216ce to 8655750 Compare May 28, 2026 07:13
Copy link
Copy Markdown
Collaborator

@AntoLC AntoLC left a comment

Choose a reason for hiding this comment

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

If you search YPROVIDER_HOST, I see some places that maybe need changes as well:

YPROVIDER_HOST=y-provider

Y_PROVIDER_API_BASE_URL: http://impress-y-provider:443/api/


A bit out of scope, but I noticed that COLLABORATION_API_URL is not in the helm charts (dev/feature).


I tried it in local as well, works great !

@lunika
Copy link
Copy Markdown
Member Author

lunika commented May 28, 2026

A bit out of scope, but I noticed that COLLABORATION_API_URL is not in the helm charts (dev/feature).

It is not because the collaborative check edition is not enabled.
I will do it in a separated commit

We want to enable the collaboration edition check in dev and feature env
@lunika lunika merged commit 4841d95 into main May 28, 2026
65 of 68 checks passed
@lunika lunika deleted the enhancement/y-provider-helm branch May 28, 2026 14:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants