-
Notifications
You must be signed in to change notification settings - Fork 31
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
Use azd binding feature to connect to services #5
base: main
Are you sure you want to change the base?
Use azd binding feature to connect to services #5
Conversation
output AZURE_RESOURCE_GROUP string = rg.name | ||
|
||
// Source resource API | ||
// output SERVICE_API_NAME string = api.outputs.SERVICE_API_NAME |
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.
Why not output it?
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.
ENV with same name exists at line 211, so it's duplicated
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 believe that is needed in that format for azd integration with aca, I'd keep all existing env vars, or do a full regression
infra/core/config/configstore.bicep
Outdated
@@ -15,8 +15,6 @@ param keyValueNames array = [] | |||
@description('Specifies the values of the key-value resources.') | |||
param keyValueValues array = [] | |||
|
|||
@description('The principal ID to grant access to the Azure App Configuration store') | |||
param principalId string |
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.
Why remove principal Id? Our design allows the user to run the application locally with their principal Id, so I think by removing this the user will no longer be able to run it locally.
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.
This rbac role assignment is raised up to main.bicep because container app env needs appConfig endpoint while appConfig needs principal Id from identity created with the container app.
The flow is changed to:
- App Config is created
- Container app is created with App Config's endpoint address filled in env variable
- This Rbac role assignment is created.
This is to avoid dependency on principal Id before the managed identity is created
src/api/src/config/index.ts
Outdated
|
||
try { | ||
logger.info("Populating environment from Azure AppConfiguration..."); | ||
const credential = new DefaultAzureCredential({ |
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.
pls use AppConfig nodejs provider instead of the standard client way to get config from AppConfig, https://learn.microsoft.com/en-us/azure/azure-app-configuration/quickstart-javascript-provider?tabs=windowscommandprompt#setting-up-the-nodejs-app
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.
fixed and will move the PR changes to the azd repo with changes to the repo template. This PR will be discard
…odo-nodejs-mongo-aca into jianc/azd-binding-sample
e10ec46
to
8024e5c
Compare
Hi @jongio , this PR will be deprecated and moved to Azure/azure-dev#3740 , the changes are made to template and repo considering there are common bicep components with a restriction to a limited scope so not to affect other existing todo project repos |
Reference azd PR for binding feature: Azure/azure-dev#3549, This is a code demo to utilize the new azd feature
binding
: