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

azdevify chat sample #1

Closed
wants to merge 3 commits into from
Closed

azdevify chat sample #1

wants to merge 3 commits into from

Conversation

v-jiaodi
Copy link

Azdevify this chat sample.

Finished:
Create the required resources with /infra, and the resources were successfully created. Run azd deploy, the apps can be deployed normally.

Questions:
Use containerapp to deploy, it cannot achieve complete interaction.

Besides, we have tried use appservice to deploy apps, but the app startup failed.
The relevant code is here: https://github.com/v-jiaodi/chat-with-your-data-solution-accelerator/tree/convert-azd-with-appservice

@jongio , @ruoccofabrizio and @gmndrg Do you have any ideas on these two issues?

@jongio
Copy link
Owner

jongio commented Oct 27, 2023

Can you include information about why it fails to start? What are the symptoms?

@@ -0,0 +1,127 @@
metadata description = 'Creates an Azure Function in an existing Azure App Service plan.'
Copy link
Owner

Choose a reason for hiding this comment

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

Did you copy this from latest? The latest is functions.bicep, not function.bicep, so I'm a little concerned that core wasn't copied from the latest. Can you double check all files are latest?

image

https://github.com/Azure/azure-dev/tree/main/templates/common/infra/bicep/core/host

@@ -0,0 +1,99 @@
{
Copy link
Owner

Choose a reason for hiding this comment

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

Please use .bicepparam instead of mian.parameters.json

@@ -104,6 +104,96 @@ Out-of-the-box, you can upload the following file types:

![A screenshot of the chat app.](./media/chat-app.png)

## Develoy with Azure Developer CLI
Copy link
Owner

Choose a reason for hiding this comment

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

Please check spelling and grammar


param identityName string
param env array
// param applicationInsightsName string
Copy link
Owner

Choose a reason for hiding this comment

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

Why comment out?

openAiName:openAiName
appSettings: union(appSettings, {
AzureWebJobsStorage: 'DefaultEndpointsProtocol=https;AccountName=${storage.name};AccountKey=${storage.listKeys().keys[0].value};EndpointSuffix=${environment().suffixes.storage}'
// AZURE_BLOB_ACCOUNT_KEY : storage.listKey().keys[0].value
Copy link
Owner

Choose a reason for hiding this comment

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

Please don't change /infra/core at all. It should be a clean copy and paste from the latest.

Copy link
Owner

@jongio jongio left a comment

Choose a reason for hiding this comment

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

Please see my comments and update the bicep to match latest core and move app specific settings to /app. Thanks

param appServicePlanId string
param keyVaultName string = ''
param managedIdentity bool = !empty(keyVaultName)
param AzureCognitiveSearch string
Copy link
Owner

Choose a reason for hiding this comment

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

These settings shouldn't be in core, but in /app.

Please keep consistent casing. Camel for params.

@zedy-wj
Copy link
Collaborator

zedy-wj commented Oct 31, 2023

@jongio We are currently investigating whether this chat sample uses appservice or containerapp, and we are currently experiencing some issues with both.

  • For appservice: We used js appservice host when deploying the frontend module. The app startup failed, because it cannot match the ts module in the code. Currently, we use ts staticwebapp host and find that it can run normally. We will try to deploy other services as appservice later.

  • For containerapp: Currently, this PR uses container app. The app can be deployed normally, but there is no data on front-end and back-end interactions.

Which one do you think is appropriate? At present, after investigation, we think that appservice is more suitable and are trying to modify it.

Regarding the comments on this PRs, if we decide to use the containerapp, we will resolve these comments ASAP .

@jongio
Copy link
Owner

jongio commented Oct 31, 2023

Which one do you think is appropriate?

Please follow what is here: https://github.com/jongio/chat-with-your-data-solution-accelerator/blob/main/infrastructure/deployment.bicep

@v-xuto
Copy link

v-xuto commented Nov 5, 2023

@jongio Sorry, we no longer use this PR, and we will close this PR. We have created a new PR #2 to replace it, and PR #2 is ready, please review.

@v-jiaodi v-jiaodi closed this Nov 6, 2023
jongio pushed a commit that referenced this pull request Feb 6, 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