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-nodejs-mongo" #3454

Closed
wants to merge 13 commits into from

Conversation

dotnet-7
Copy link

@dotnet-7 dotnet-7 commented Feb 29, 2024

Update the infra/core modules to AVM modules for todo-nodejs-mongo.

1. Modules not replaced by AVM

2. Modules replaced by AVM

  • App Service
  • Key Vault
  • App Service Plan

@jongio for notification.

@dotnet-7 dotnet-7 marked this pull request as ready for review February 29, 2024 10:13
@vhvb1989
Copy link
Member

/azp run azure-dev - repoman

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Copy link
Member

@vhvb1989 vhvb1989 left a comment

Choose a reason for hiding this comment

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

LGTM and tests passed

Copy link
Contributor

@wbreza wbreza left a comment

Choose a reason for hiding this comment

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

I have no issue adopting AVM and migrating away from our home grown core modules.
If we move forward we should update all the templates and deprecate the core modules.

One way to approach this would be to PR into a feature branch and then merge the feature branch back into main when these updates are completed.

@jongio jongio marked this pull request as draft February 29, 2024 22:56
@vhvb1989
Copy link
Member

vhvb1989 commented Mar 1, 2024

I have no issue adopting AVM and migrating away from our home grown core modules. If we move forward we should update all the templates and deprecate the core modules.

One way to approach this would be to PR into a feature branch and then merge the feature branch back into main when these updates are completed.

Repoman does the branch on each template automatically. Shouldn't it be a duplication to branch azure-dev as well?
Keep in mind that we are updating the todo-templates, while the core modules are still available for anyone from the repo. We can keep those around for a while

Comment on lines 207 to 208
webFrontendUrl: 'https://${web.outputs.defaultHostname}'
apiBackendUrl: 'https://${api.outputs.defaultHostname}'
Copy link
Member

Choose a reason for hiding this comment

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

Let's get this format in a variable b/c it is used in multiple places.

appInsightResourceId: resourceId(subscription().subscriptionId, rg.name, 'Microsoft.insights/components', monitoring.outputs.applicationInsightsName)
siteConfig: {
windowsFxVersion: 'node|18-lts'
appCommandLine: './entrypoint.sh -o ./env-config.js && pm2 serve /home/site/wwwroot --no-daemon --spa'
Copy link
Member

Choose a reason for hiding this comment

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

@vhvb1989 are you moving away from entrypoint.sh?

Copy link
Member

Choose a reason for hiding this comment

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

WIP: #3171

@jongio
Copy link
Member

jongio commented Mar 8, 2024

Please try to make this work without any modules from infra/core. I want to see what all the gaps look like

@v-jiaodi
Copy link
Member

v-jiaodi commented Mar 13, 2024

@jongio I have updated the code, please review.

There are two questions that need to be clarified:

  1. For applicationInsightsDashboard, Microsoft.ApiManagement/service/loggers and Microsoft.ApiManagement/service/apis/diagnostics, since it was not implemented in AVM, we have retained the original implementation.
  2. Remove apim service sku config. For Consumption SKU Type capacity must be specified as 0, but in AVM , only 1 and 2 are allowed: https://github.com/Azure/bicep-registry-modules/blob/main/avm/res/api-management/service/main.bicep#L68
  3. For cosmosConnectString, we store in keyVault by listConnectionStrings(resourceId, '2022-08-15').connectionStrings[0].connectionString

@v-xuto
Copy link
Member

v-xuto commented Mar 15, 2024

/azp run azure-dev - repoman

@Azure Azure deleted a comment from azure-sdk Mar 15, 2024
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@Azure Azure deleted a comment from azure-pipelines bot Mar 15, 2024
@jongio
Copy link
Member

jongio commented Mar 26, 2024

@jongio I have updated the code, please review.

There are two questions that need to be clarified:

  1. For applicationInsightsDashboard, Microsoft.ApiManagement/service/loggers and Microsoft.ApiManagement/service/apis/diagnostics, since it was not implemented in AVM, we have retained the original implementation.
  2. Remove apim service sku config. For Consumption SKU Type capacity must be specified as 0, but in AVM , only 1 and 2 are allowed: https://github.com/Azure/bicep-registry-modules/blob/main/avm/res/api-management/service/main.bicep#L68
  3. For cosmosConnectString, we store in keyVault by listConnectionStrings(resourceId, '2022-08-15').connectionStrings[0].connectionString

Can you please reference the issues that we have filed for each of the above?

@v-jiaodi
Copy link
Member

@jongio

  1. Monitoring (app insights) : Missing the feature Microsoft.Portal/dashboards, since it was not implemented in AVM, we have retained the original implementation. [AVM Module Issue]: Application Insight Module should add the creation of 'Microsoft.Portal/dashboards' bicep-registry-modules#1130
  2. APIM: Missing features Microsoft.ApiManagement/service/loggers and Microsoft.ApiManagement/service/apis/diagnostics.
    [AVM Module Issue]: API Management Module should add the creation of 'Microsoft.ApiManagement/service/loggers' and 'Microsoft.ApiManagement/service/apis/diagnostics' bicep-registry-modules#1124

@v-xuto
Copy link
Member

v-xuto commented May 29, 2024

/azp run azure-dev - repoman

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@dotnet-7 dotnet-7 force-pushed the avm branch 3 times, most recently from d858ca6 to 97dd7ea Compare May 29, 2024 09:14
@Menghua1 Menghua1 changed the title Update the infra/core modules to AVM modules Update the infra/core modules to AVM modules of "todo-nodejs-mongo" May 29, 2024
@v-xuto
Copy link
Member

v-xuto commented May 30, 2024

/azp run azure-dev - repoman

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@azure-sdk
Copy link
Collaborator

Repoman Generation Results

Repoman pushed changes to remotes for the following projects:

Project: todo-nodejs-mongo

Remote: azure-samples-staging

Branch: pr/3454

You can initialize this project with:

azd init -t Azure-Samples/todo-nodejs-mongo -b pr/3454

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.

9 participants