-
Notifications
You must be signed in to change notification settings - Fork 250
Create all Azure resource in the same region of the resource group #184
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
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.
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
@petetian please read the following Contributor License Agreement(CLA). If you agree with the CLA, please reply with the following information.
Contributor License AgreementContribution License AgreementThis Contribution License Agreement (“Agreement”) is agreed to by the party signing below (“You”),
|
# To give the Container Apps environment secure access to the container registry, we’ll create a user managed identity and assign it the required privileges to use the images stored in your Azure Container Registry. | ||
|
||
APPNAME=petclinic | ||
RESOURCE_GROUP=$(az group list --query "[?contains(name, 'petclinic')].{Name:name}[0]" -o tsv) |
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.
'petclinic' -> '$APPNAME'
|
||
UNIQUEID=$(openssl rand -hex 3) | ||
APPNAME=petclinic | ||
RESOURCE_GROUP=$(az group list --query "[?contains(name, 'petclinic')].{Name:name}[0]" -o tsv) |
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.
when multiple members share the same subscription, this would cause some conflicts on same resource group.
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.
same for the acr, aca environment list & check
# Check if a container registry containing the string $APPNAME already exists | ||
EXISTING_ACR=$(az acr list --resource-group $RESOURCE_GROUP --query "[?contains(name, '$APPNAME')].{Name:name}[0]" -o tsv) | ||
|
||
UNIQUEID=$(openssl rand -hex 3) |
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.
renew the UNIQUEID will cause mismatch in the later steps.
|
||
# Resource Group | ||
|
||
# Random regions for MySQL server | ||
LOCATION=$(random_element australiaeast brazilsouth eastasia eastus2 japaneast southindia swedencentral westus) |
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.
Here are the problems with these regions:
- OpenAI not supported in eastasia
- Managed Grafana not supported in southindia, prefer use centralindia
See the differences between AI_LOCATION, SQL_LOCATION, GRAFANA_LOCATION
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.
BTW, I don't think the potential regional latency is matter in this lab environment
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.
Please sync the latest code and pay attention to my previous comments
…plication config yaml
@sonwan2020 and @petetian request you to advance and merge this ASAP. Mark Heckler ran into similar challenges |
@selvasingh which challenge do you refer to?
|
@sonwan2020: please review the PR. Thanks! |
@@ -0,0 +1,9 @@ | |||
#!/bin/bash |
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 some environment like Github codespace, we may fail to get the AAD_USER_ID, we should check if the AAD_USER_ID is valid before continue
|
||
UNIQUEID=$(openssl rand -hex 3) | ||
export APPNAME=petclinic | ||
export RESOURCE_GROUP=$(az group list --query "[?contains(name, 'petclinic')].{Name:name}[0]" -o tsv) |
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.
replace the petclinc to $APPNAME
export RESOURCE_GROUP=$(az group list --query "[?contains(name, 'petclinic')].{Name:name}[0]" -o tsv) | ||
|
||
if [ -z "$RESOURCE_GROUP" ]; then | ||
RESOURCE_GROUP=rg-$APPNAME-$UNIQUEID |
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.
we will have user to set the UNIQUEID in profile and import those settings first.
we run "export RESOURCE_GROUP=rg-$APPNAME-$UNIQUEID" at the beginning.
In the current process,
- there will be conflict on resource group name
- we make sure we can rerun the script
echo "Provisioning in resource group [$RESOURCE_GROUP]..." | ||
fi | ||
|
||
az configure --default group=$RESOURCE_GROUP |
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.
we may ask user to input the subscription.
az account set -s SUB
or az configure --default subscription=xxx
# Check if a container registry containing the string $APPNAME already exists | ||
MYACR=$(az acr list --resource-group $RESOURCE_GROUP --query "[?contains(name, '$APPNAME')].{Name:name}[0]" -o tsv) | ||
|
||
CR_UNIQUEID=$(openssl rand -hex 3) |
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 prefer this way: user set the unique id in profile and we use the id for new resources
I am OK about the scripts, but I am concerned about the process. let me have another discussion |
Purpose
create-azure-resources.sh was choosing SQL, AI and GRAFANA LOCATION randomly, which may allocate these resources in different locations. Having discussion with course leaders, the consensus is that resource group location can be random, but the resources within the resource group should be in the same location, to avoid network latency.
Minor change in azure-resource.profile: add instruction taken from learner's manual as comment, including generate unique-id, and query azure subscription-id in comments, for learner's reference
Does this introduce a breaking change?
Pull Request Type
What kind of change does this Pull Request introduce?
Move variable LOCATION from azure-resource.profile to create-azure-resources.sh, and generates randomly, and every azure service within use the same location setting, so that all resources are created in the same region.
How to Test
What to Check
Verify if all the services are created in the same Azure region.
Other Information