-
Notifications
You must be signed in to change notification settings - Fork 214
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add schema support for cosmos db #4780
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks good. Left some minor comments.
@@ -407,6 +457,12 @@ module {{bicepName .Name}} 'br/public:avm/res/app/container-app:0.8.0' = { | |||
secretRef: 'mongodb-url' | |||
} | |||
{{- end}} | |||
{{- if .DbCosmos}} | |||
{ | |||
name: 'AZURE_COSMOSDB_ENDPOINT' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does AZURE_COSMOS_ENDPOINT
seem any better here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, AZURE_COSMOS_ENDPOINT
is better.
cli/azd/pkg/project/resources.go
Outdated
ContainerName string `yaml:"containerName,omitempty"` | ||
PartitionKeyPaths []string `yaml:"partitionKeyPaths,omitempty"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does Name
and PartitionKeys
sound any better?
cli/azd/pkg/project/resources.go
Outdated
Name string `yaml:"containerName,omitempty"` | ||
PartitionKeys []string `yaml:"partitionKeyPaths,omitempty"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the YAML schema, does name
and partitionKeys
feel nicer?
containers: [ | ||
{{- range .DbCosmos.Containers}} | ||
{ | ||
name: '{{ .ContainerName }}' | ||
paths: [ | ||
{{- range $path := .PartitionKeyPaths}} | ||
'{{ $path }}' | ||
{{- end}} | ||
] | ||
} | ||
{{- end}} | ||
] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've had time to look into this more, and I'm wondering if we should not specify any container properties. For non-Java languages, this is usually created as a dataplane operation within the SDK clients.
I think there's a similar concept in Azure Spring Cloud for CosmosDB, where the annotation would create the container on boot time. Would you be able to confirm and see if we can avoid adding this entirely?
Azure Dev CLI Install InstructionsInstall scriptsMacOS/Linux
bash:
pwsh:
WindowsPowerShell install
MSI install
Standalone Binary
MSI
Documentationlearn.microsoft.com documentationtitle: Azure Developer CLI reference
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks good. Final question to address regarding if we need to support containers at all (or leave it to the application).
Address #4671, this PR will add the resource support of Azure Cosmos DB.