Skip to content

Conversation

jaraya-mycarrier
Copy link
Contributor

No description provided.

Copy link
Contributor

@Copilot 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 provisioning Azure Blob Storage resources through Crossplane by introducing a comprehensive claim template and associated test suite. The implementation provides flexible configuration options for both existing and new Azure storage accounts with auto-generated naming conventions.

Key changes:

  • Added infrastructure configuration section to values.yaml with detailed Azure storage account parameters
  • Created Crossplane claim template for Azure Blob Storage with fallback logic for resource naming
  • Implemented comprehensive test suite covering various configuration scenarios

Reviewed Changes

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

File Description
charts/mycarrier-helm/values.yaml Added infrastructure.azure.storage configuration section with extensive documentation and examples
charts/mycarrier-helm/templates/crossplane/azure/storage/claim.yaml New Crossplane claim template with conditional logic for storage account and resource group provisioning
charts/mycarrier-helm/tests/crossplane_azure_storage_test.yaml Test suite covering default naming, minimal configuration, empty configuration, and label verification scenarios

name: {{ $instance.existingStorageAccount.name }}
{{- else if $instance.newStorageAccount }}
newStorageAccount:
name: {{ $instance.newStorageAccount.name | default (printf "st%s%s" ($.Values.global.appStack | replace "-" "" | lower) ($.Values.environment.name | replace "-" "" | lower)) }}
Copy link

Copilot AI Aug 20, 2025

Choose a reason for hiding this comment

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

The storage account name generation logic is duplicated on lines 11, 30, and 44. This violates the DRY principle and makes maintenance difficult. Consider extracting this logic into a helper template in _helpers.tpl.

Copilot uses AI. Check for mistakes.

{{- if $instance.writeConnectionSecretsToRef }}
# writeConnectionSecretsToRef:
# name: {{ $instance.writeConnectionSecretsToRef.name | default (printf "%s-connection" ($instance.name | default (printf "st-%s-%s" $.Values.global.appStack $.Values.environment.name))) }}
# namespace: {{ $instance.writeConnectionSecretsToRef.namespace | default (printf "%s-%s" $.Values.environment.name $.Values.global.appStack) }}
Copy link

Copilot AI Aug 20, 2025

Choose a reason for hiding this comment

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

The commented-out writeConnectionSecretsToRef section should either be implemented or removed. Having commented code in production templates can be confusing and adds maintenance overhead.

Suggested change
# namespace: {{ $instance.writeConnectionSecretsToRef.namespace | default (printf "%s-%s" $.Values.environment.name $.Values.global.appStack) }}
writeConnectionSecretsToRef:
name: {{ $instance.writeConnectionSecretsToRef.name | default (printf "%s-connection" ($instance.name | default (printf "st-%s-%s" $.Values.global.appStack $.Values.environment.name))) }}
namespace: {{ $instance.writeConnectionSecretsToRef.namespace | default (printf "%s-%s" $.Values.environment.name $.Values.global.appStack) }}

Copilot uses AI. Check for mistakes.

)
)
)
}}
Copy link

Copilot AI Aug 20, 2025

Choose a reason for hiding this comment

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

[nitpick] The nested default expressions for name generation (lines 7-15) create complex and hard-to-read logic. This could be simplified by extracting the logic into a helper template or using a more straightforward conditional structure.

Suggested change
}}
{{- /* Helper template for Azure storage account name generation */ -}}
{{- define "azure.storage.account.name" -}}
{{- $instance := . -}}
{{- if $instance.name -}}
{{ $instance.name }}
{{- else if $instance.newStorageAccount.name -}}
{{ $instance.newStorageAccount.name }}
{{- else if $instance.existingStorageAccount.name -}}
{{ $instance.existingStorageAccount.name }}
{{- else -}}
{{ printf "st%s%s" ($.Values.global.appStack | replace "-" "") ($.Values.environment.name | replace "-" "") }}
{{- end -}}
{{- end }}
{{- if .Values.infrastructure.azure.storage.accounts }}
{{- range $instance := .Values.infrastructure.azure.storage.accounts }}
---
apiVersion: azure.platform.io/v1alpha1
kind: AzureBlobStorage
metadata:
name: {{ template "azure.storage.account.name" $instance }}

Copilot uses AI. Check for mistakes.

{{- else }}
# Default: create new resource group with auto-generated name (Azure compliant: 1-90 chars, alphanumeric, hyphens, underscores, periods, parentheses)
newResourceGroup:
name: {{ ($instance.newResourceGroup).name | default (printf "rg-%s-%s" $.Values.global.appStack $.Values.environment.name) }}
Copy link

Copilot AI Aug 20, 2025

Choose a reason for hiding this comment

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

The resource group name generation logic is also duplicated. Consider creating a helper template for consistent resource naming patterns across the chart.

Suggested change
name: {{ ($instance.newResourceGroup).name | default (printf "rg-%s-%s" $.Values.global.appStack $.Values.environment.name) }}
name: {{ ($instance.newResourceGroup).name | default (include "azure.resourceGroupName" (dict "appStack" $.Values.global.appStack "environmentName" $.Values.environment.name)) }}

Copilot uses AI. Check for mistakes.

@jaraya-mycarrier
Copy link
Contributor Author

Improvements based on Copilot review:

  • Extracted duplicated storage account name generation logic into reusable helper templates
  • Extracted duplicated resource group name generation logic into helper template
  • Simplified complex nested default expressions for better readability
  • Moved Crossplane-specific helpers to dedicated _helpers.crossplane.tpl file
  • Removed unused commented code (writeConnectionSecretsToRef section)

@jaraya-mycarrier jaraya-mycarrier merged commit 483d96f into main Aug 26, 2025
1 check failed
@jaraya-mycarrier jaraya-mycarrier deleted the crossplane branch August 26, 2025 14:34
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.

1 participant