Skip to content

ITEP-81092: Added mapping service to helm deployment#589

Merged
saratpoluri merged 15 commits intorelease-2025.2from
mapping_service_helm_chart_deployment
Nov 21, 2025
Merged

ITEP-81092: Added mapping service to helm deployment#589
saratpoluri merged 15 commits intorelease-2025.2from
mapping_service_helm_chart_deployment

Conversation

@Irakus
Copy link
Copy Markdown
Contributor

@Irakus Irakus commented Nov 17, 2025

📝 Description

Added mapping service to helm deployyment. It's disabled by default.

✨ Type of Change

Select the type of change your PR introduces:

  • 🐞 Bug fix – Non-breaking change which fixes an issue
  • 🚀 New feature – Non-breaking change which adds functionality
  • 🔨 Refactor – Non-breaking change which refactors the code base
  • 💥 Breaking change – Changes that break existing functionality
  • 📚 Documentation update
  • 🔒 Security update
  • 🧪 Tests
  • 🚂 CI

🧪 Testing Scenarios

Describe how the changes were tested and how reviewers can test them too:

  • ✅ Tested manually
  • 🤖 Ran automated end-to-end tests

✅ Checklist

Before submitting the PR, ensure the following:

  • 🔍 PR title is clear and descriptive
  • 📝 For internal contributors: If applicable, include the JIRA ticket number (e.g., ITEP-123456) in the PR title. Do not include full URLs
  • 💬 I have commented my code, especially in hard-to-understand areas
  • 📄 I have made corresponding changes to the documentation
  • ✅ I have added tests that prove my fix is effective or my feature works

@Irakus Irakus marked this pull request as ready for review November 19, 2025 13:41
Copy link
Copy Markdown
Contributor

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 adds support for a new mapping service to the Helm deployment, configured to be disabled by default. The changes include creating deployment, service, and certificate templates for the mapping service, along with necessary configuration values and integration points.

Key Changes:

  • Added Helm templates for mapping service deployment, service, and certificate resources
  • Integrated mapping service URL into web-app deployment when enabled
  • Standardized image reference format in cluster-analytics deployment

Reviewed Changes

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

Show a summary per file
File Description
kubernetes/scenescape-chart/values.yaml Adds configuration values for the mapping service (disabled by default)
kubernetes/scenescape-chart/templates/mapping/deployment.yaml Creates deployment template for mapping service with security contexts and volume mounts
kubernetes/scenescape-chart/templates/mapping/service.yaml Defines Kubernetes service and optional load balancer for mapping service
kubernetes/scenescape-chart/templates/mapping/certificate.yaml Configures TLS certificate for mapping service
kubernetes/scenescape-chart/templates/web-app/deployment.yaml Adds conditional mapping service URL environment variable
kubernetes/scenescape-chart/templates/_helpers.tpl Includes mapping service certificate in shared certs volume
kubernetes/scenescape-chart/templates/cluster-analytics/deployment.yaml Fixes image reference format and renames pullPolicy to imagePullPolicy

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

Comment on lines +135 to +138
mapping:
enabled: false
image: intel/scenescape-mapping
pullPolicy: IfNotPresent
Copy link

Copilot AI Nov 20, 2025

Choose a reason for hiding this comment

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

The image reference format is inconsistent with other services in the deployment. For consistency with the cluster-analytics service (which uses separate repository and image fields), consider following the same pattern or explicitly including the full registry path if intentionally different.

Copilot uses AI. Check for mistakes.
Comment on lines +5 to +8
apiVersion: cert-manager.io/v1
kind: Certificate
metadata:
name: {{ .Release.Name }}-mapping-cert
Copy link

Copilot AI Nov 20, 2025

Choose a reason for hiding this comment

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

The certificate resource is not conditionally created based on .Values.mapping.enabled. Unlike deployment.yaml and service.yaml which check this flag, this certificate will always be created even when the mapping service is disabled. Add {{- if .Values.mapping.enabled }} at line 5 and {{- end }} at the end of the file.

Copilot uses AI. Check for mistakes.
Comment on lines +58 to +64
- secret:
name: {{ .Release.Name }}-mapping-tls
items:
- key: tls.key
path: scenescape-mapping.key
- key: tls.crt
path: scenescape-mapping.crt
Copy link

Copilot AI Nov 20, 2025

Choose a reason for hiding this comment

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

The mapping certificate secret is unconditionally added to the shared certs volume, but the certificate itself may not exist when mapping is disabled. This will cause volume mount failures. Wrap this secret reference in a conditional check: {{- if .Values.mapping.enabled }}.

Copilot uses AI. Check for mistakes.
@saratpoluri saratpoluri changed the title Added mapping service to helm deployment ITEP-81092: Added mapping service to helm deployment Nov 20, 2025
@saratpoluri saratpoluri force-pushed the mapping_service_helm_chart_deployment branch from eaa904c to cdfdeca Compare November 20, 2025 01:17
Comment thread kubernetes/scenescape-chart/templates/mapping/deployment.yaml Outdated
@saratpoluri saratpoluri enabled auto-merge (squash) November 20, 2025 17:13
@saratpoluri saratpoluri merged commit 4a8aea9 into release-2025.2 Nov 21, 2025
25 checks passed
@saratpoluri saratpoluri deleted the mapping_service_helm_chart_deployment branch November 21, 2025 01:27
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.

5 participants