This repository was archived by the owner on Sep 9, 2026. It is now read-only.
OSAC-1628: Add required externalHostname and internalHostname to umbrella chart - #295
Merged
openshift-merge-bot[bot] merged 1 commit intoJun 18, 2026
Conversation
…umbrella chart The fulfillment-service subchart is making `externalHostname` and `internalHostname` mandatory because TLS certificates must include the correct hostnames and OpenShift cannot auto-generate Route hosts when certificates are pre-provisioned. These values cannot be auto-calculated at template time since the cluster's ingress domain is not available without cluster access. Add both parameters to the umbrella chart's default values, example values, and JSON schema (marked as required with `minLength: 1`). Populate all CI values files with example hostnames so that linting and integration workflows continue to pass. Update the helm-lint and helm-integration workflows to supply dummy hostnames when templating with environment values files, which intentionally leave the fields empty for operators to fill at deploy time. Related: https://redhat.atlassian.net/browse/OSAC-1628 Ref: osac-project/fulfillment-service#727 Assisted-by: Cursor Signed-off-by: Juan Hernandez <juan.hernandez@redhat.com>
|
@jhernand: This pull request references OSAC-1628 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the bug to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
WalkthroughTwo new required Helm values — ChangesHostname Fields: Schema, Values, CI Fixtures, Workflows, and Docs
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 11✅ Passed checks (11 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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 `@charts/osac/values.schema.json`:
- Around line 97-108: The externalHostname and internalHostname fields in the
schema only validate minLength: 1, which allows syntactically invalid hostnames
to pass validation and fail later during Route or certificate creation. Add a
pattern property to both externalHostname and internalHostname fields with a
valid hostname regex pattern (RFC-compliant FQDN pattern), and also add a
maxLength property to enforce reasonable hostname length limits. This will catch
invalid hostnames early at schema validation time rather than at deployment
time.
🪄 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: osac-project/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 441dae1c-e47b-4f0c-947c-222ef379b0a0
📒 Files selected for processing (13)
.github/workflows/helm-integration.yaml.github/workflows/helm-lint.yamlcharts/osac/ci/bundled-postgres-values.yamlcharts/osac/ci/default-values.yamlcharts/osac/ci/full-values.yamlcharts/osac/ci/no-aap-values.yamlcharts/osac/values-example.yamlcharts/osac/values.schema.jsoncharts/osac/values.yamldocs/helm-deployment-guide.mdvalues/caas-ci.yamlvalues/development.yamlvalues/vmaas-ci.yaml
Comment on lines
+97
to
+108
| "required": ["externalHostname", "internalHostname", "auth", "certs"], | ||
| "properties": { | ||
| "externalHostname": { | ||
| "type": "string", | ||
| "minLength": 1, | ||
| "description": "Hostname for the external API Route" | ||
| }, | ||
| "internalHostname": { | ||
| "type": "string", | ||
| "minLength": 1, | ||
| "description": "Hostname for the internal API Route" | ||
| }, |
There was a problem hiding this comment.
Strengthen hostname validation in schema.
minLength: 1 only checks non-empty values, so syntactically invalid hostnames can pass validation and then fail later at Route/certificate creation. Add a hostname pattern (and preferably max length) for both fields.
Proposed fix
"externalHostname": {
"type": "string",
"minLength": 1,
+ "maxLength": 253,
+ "pattern": "^([a-z0-9]([-a-z0-9]*[a-z0-9])?)(\\.([a-z0-9]([-a-z0-9]*[a-z0-9])?))*$",
"description": "Hostname for the external API Route"
},
"internalHostname": {
"type": "string",
"minLength": 1,
+ "maxLength": 253,
+ "pattern": "^([a-z0-9]([-a-z0-9]*[a-z0-9])?)(\\.([a-z0-9]([-a-z0-9]*[a-z0-9])?))*$",
"description": "Hostname for the internal API Route"
},📝 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
| "required": ["externalHostname", "internalHostname", "auth", "certs"], | |
| "properties": { | |
| "externalHostname": { | |
| "type": "string", | |
| "minLength": 1, | |
| "description": "Hostname for the external API Route" | |
| }, | |
| "internalHostname": { | |
| "type": "string", | |
| "minLength": 1, | |
| "description": "Hostname for the internal API Route" | |
| }, | |
| "required": ["externalHostname", "internalHostname", "auth", "certs"], | |
| "properties": { | |
| "externalHostname": { | |
| "type": "string", | |
| "minLength": 1, | |
| "maxLength": 253, | |
| "pattern": "^([a-z0-9]([-a-z0-9]*[a-z0-9])?)(\\.([a-z0-9]([-a-z0-9]*[a-z0-9])?))*$", | |
| "description": "Hostname for the external API Route" | |
| }, | |
| "internalHostname": { | |
| "type": "string", | |
| "minLength": 1, | |
| "maxLength": 253, | |
| "pattern": "^([a-z0-9]([-a-z0-9]*[a-z0-9])?)(\\.([a-z0-9]([-a-z0-9]*[a-z0-9])?))*$", | |
| "description": "Hostname for the internal API Route" | |
| }, |
🤖 Prompt for 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.
In `@charts/osac/values.schema.json` around lines 97 - 108, The externalHostname
and internalHostname fields in the schema only validate minLength: 1, which
allows syntactically invalid hostnames to pass validation and fail later during
Route or certificate creation. Add a pattern property to both externalHostname
and internalHostname fields with a valid hostname regex pattern (RFC-compliant
FQDN pattern), and also add a maxLength property to enforce reasonable hostname
length limits. This will catch invalid hostnames early at schema validation time
rather than at deployment time.
adriengentil
approved these changes
Jun 18, 2026
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: adriengentil, jhernand The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
This was referenced Jun 19, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
externalHostnameandinternalHostnameas required parameters in the umbrella chart'svalues, schema, and CI values files. These are needed because the fulfillment-service subchart
is making them mandatory (see OSAC-1628: Make
externalHostnameandinternalHostnamemandatory fulfillment-service#727) since TLS certificatesmust include the correct hostnames.
with environment values files.
Test plan
helm templatepasses with the updated CI values files.Related: https://redhat.atlassian.net/browse/OSAC-1628
Ref: osac-project/fulfillment-service#727
Summary by CodeRabbit
New Features
externalHostnameandinternalHostnameconfiguration fields for API service endpoints.Documentation
Chores