feat: add option to use standard helm naming conventions

[alexstojda]

This pull request introduces support for standard Helm naming conventions in the charts/pdp Helm chart while preserving backward compatibility with the existing naming scheme. It adds a feature flag (useStandardHelmNamingConventions) to toggle between the naming conventions.

This is the standard naming convention of resources virtually all helm charts use. Defining static names as is currently done presents the risk of potential name collisions with other resources and/or other PDP deployments in the same namespace.

[alexstojda]

@EliMoshkovich please have a look when you can :)

[alexstojda]

@EliMoshkovich could you have a look please?

[EliMoshkovich]

Hey @alexstojda ! Sorry I missed your earlier messages.
I’m reviewing the PR now 👀

[EliMoshkovich]

Hey @alexstojda!

Thanks for this PR! The implementation looks solid and follows Helm best practices. I did a thorough review and found one critical issue that needs to be fixed before merge.

🚨 Critical: ConfigMap Reference Mismatch

When this bug occurs: Only when both useStandardHelmNamingConventions: true and pdp.logs_forwarder.enabled: true are set together.

The issue:

• ConfigMap is created with name: {release-name}-pdp-fluentbit-config (line 6 in logs-forwarder-cm.yaml)
• Deployment still references hardcoded: fluentbit-config (line 144 in deployment.yaml)
• Result: Pod fails to mount ConfigMap because the name doesn't match

Verified with helm template:

$ helm template test-release . --set useStandardHelmNamingConventions=true --set pdp.logs_forwarder.enabled=true

# Creates ConfigMap:
  name: test-release-pdp-fluentbit-config

# But references:
  name: fluentbit-config  # ❌ Not found!

Fix needed in deployment.yaml line 144:

      volumes:
      {{- if .Values.pdp.logs_forwarder.enabled }}
        - name: fluent-bit-config
          configMap:
            {{- if .Values.useStandardHelmNamingConventions }}
            name: {{ include "..fullname" . }}-fluentbit-config
            {{- else }}
            name: fluentbit-config
            {{- end }}
        - name: logs
          emptyDir: {}

---

💡 Optional Suggestions (Non-blocking)

These are nice-to-haves that could be addressed in a follow-up if you prefer:

1. Add fullnameOverride to values.yaml - It's already handled in _helpers.tpl, just needs documentation
2. Consider standard helper naming - Most Helm charts use pdp.fullname instead of ..fullname (purely cosmetic)

---

✅ What Looks Great

• Excellent documentation explaining the feature in values.yaml
• Smart backward compatibility with sensible defaults
• Proper handling of edge cases (name deduplication)
• Clean, focused implementation

Once the ConfigMap fix is in, this is ready to merge! 🚀

[alexstojda]

Thanks @EliMoshkovich, I implemented the changes you recommended

[Copilot]

Pull request overview

This PR introduces a feature flag to enable standard Helm naming conventions in the charts/pdp chart while maintaining backward compatibility with existing static resource names. The change allows users to opt into the standard {release-name}-{chart-name} naming pattern, preventing potential name collisions when multiple PDP instances are deployed in the same namespace.

Changes:

• Added useStandardHelmNamingConventions feature flag (default: false) to preserve backward compatibility
• Implemented standard Helm helper templates (pdp.name and pdp.fullname) with name truncation support
• Updated resource templates to conditionally apply naming conventions based on the feature flag

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
charts/pdp/values.yaml	Adds feature flag, nameOverride/fullnameOverride fields, and comprehensive documentation explaining naming behavior
charts/pdp/templates/_helpers.tpl	Implements standard Helm naming helper templates and updates secret name resolution logic
charts/pdp/templates/service.yaml	Conditionally applies standard naming to Service resources
charts/pdp/templates/deployment.yaml	Conditionally applies standard naming to Deployment resources and updates ConfigMap reference
charts/pdp/templates/poddisruptionbudget.yaml	Conditionally applies standard naming to PodDisruptionBudget resources
charts/pdp/templates/logs-forwarder-cm.yaml	Conditionally applies standard naming to ConfigMap resources

---

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

[Copilot]

The ConfigMap name reference is unconditionally changed to use the standard naming convention, but this will break existing deployments when useStandardHelmNamingConventions is false (the default). This should be wrapped in the same conditional logic used in other templates to maintain backward compatibility.

[zeevmoney]

Thank you for the contribution, see comments.

[zeevmoney]

add to values.yaml that selector labels won't change with the feature flag.
Consider also adding "app.kubernetes.io/name" and "app.kubernetes.io/instance" labels as standard Helm practice.

[zeevmoney]

add the condition here also for "useStandardHelmNamingConventions"

[zeevmoney]

Suggested change

	name: {{ include "pdp.fullname" . }}
	name: {{ include "pdp.fullname" . }}-pdb

[zeevmoney]

Excellent docs please add te fluentbit-config ConfigMap naming (fluentbit-config vs {fullname}-fluentbit-config)

[zeevmoney]

These are standard Helm overrides but they're only used when
useStandardHelmNamingConventions=true.
Explain it in a comment please.