Skip to content
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

Update the infra/core modules to AVM modules of "todo-csharp-sql" #3620

Closed
wants to merge 7 commits into from

Conversation

Menghua1
Copy link
Member

@Menghua1 Menghua1 commented Mar 29, 2024

Update the infra/core modules to AVM modules for todo-csharp-sql.

1. Modules not replaced by AVM

2. Modules replaced by AVM

  • App Service
  • Key Vault
  • App Service Plan
  • SqlService
  • Azure loganalytics
  • Azure applicationInsights

@jongio for notification.

@Menghua1
Copy link
Member Author

Menghua1 commented Apr 1, 2024

@jongio According to your comments, the code has been changed. Please review again, thanks.

Comment on lines 17 to 22
resource restApi 'Microsoft.ApiManagement/service/apis@2021-12-01-preview' existing = {
name: apiName
parent: apimService
}

resource apiDiagnostics 'Microsoft.ApiManagement/service/apis/diagnostics@2021-12-01-preview' = {
Copy link
Member

@jongio jongio Apr 15, 2024

Choose a reason for hiding this comment

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

I may have asked this before but, I'm wondering why this type apimanagement/service/apis/diag isn't in infra/core.

Copy link
Member Author

Choose a reason for hiding this comment

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

I may have asked this before but, I'm wondering why this type apimanagement/service/apis/diag isn't in infra/core.

This apimanagement/service/apis/diag is not originally in infra/core, but in app/apim-api.bicep, and there is no equivalent function in AVM. We have already sent an AVM issue: Azure/bicep-registry-modules#1124.

name: 'keyvault'
scope: rg
params: {
name: !empty(keyVaultName) ? keyVaultName : '${abbrs.keyVaultVaults}${resourceToken}'
location: location
tags: tags
principalId: principalId
Copy link
Member

Choose a reason for hiding this comment

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

How will user be able to run the app locally without this?

Copy link
Member Author

Choose a reason for hiding this comment

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

I added permissions elsewhere instead, the link is here: https://github.com/Azure/azure-dev/pull/3620/files/a35fc18c3ab15d73fc88c49fd0ccb75fc029eb82#diff-f1fba4c513e3fd25a2a0c835cbfe410e23057d6680763ac3f0bd1ebdf630b8abR132.
If you want to add the permission of principalId at the current location, it is also possible. We can modify the relevant code to complete it.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, as-is is fine

@Menghua1 Menghua1 changed the title Update the infra/core modules to AVM modules Update the infra/core modules to AVM modules of "todo-csharp-sql" May 15, 2024
@azure-sdk
Copy link
Collaborator

Repoman Generation Results

Repoman pushed changes to remotes for the following projects:

Project: todo-csharp-sql

Remote: azure-samples-staging

Branch: pr/3620

You can initialize this project with:

azd init -t Azure-Samples/todo-csharp-sql -b pr/3620

View Changes | Compare Changes


@v-xuto v-xuto closed this Jun 4, 2024
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.

4 participants