-
Notifications
You must be signed in to change notification settings - Fork 27
Task7 azurestoragescript #8
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
Task7 azurestoragescript #8
Conversation
|
Looks good reviewed by 1jc Team2. Ready for Pull and Merge. |
1jc checked individual coding for task several times 1jc |
Write-Host "Create Azure Storage Account" | ||
|
||
# Name of resource group | ||
$resourceGroup = "nsc-ad440-winter2021-Th-StorageRG" |
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.
@theanticrumpet Hard coding the name of the resource group is not a group practice. This makes the script inflexible - i.e. once you use it you cannot reuse it again if the resource groups. It is better if you make those at least input parameters for the script.
CC:// @1jc
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 was a little hesitant to do what you suggested in slack where I take multiple inputs and combine them into the RG name since that would just lead to a bunch of similarly named but different RGs. Would prompting for the RG name be sufficient?
# Name of resource group | ||
$resourceGroup = "nsc-ad440-winter2021-Th-StorageRG" | ||
# Location for Azure resources | ||
$location = "westus2" |
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.
@theanticrumpet Same comment as above
CC:// @1jc
# Location for Azure resources | ||
$location = "westus2" | ||
#Name of storage account | ||
$storageAccountName = "nscad440thsa" |
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.
@theanticrumpet Same comment as above
CC:// @1jc
$storageAccountName = "nscad440thsa" | ||
|
||
#Check if the resource group exists | ||
Get-AzResourceGroup -Name $resourceGroup -ErrorVariable noRG -ErrorAction SilentlyContinue |
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.
@theanticrumpet I do not see a logic to sign into the Azure account. This line will fail if I try to run the script. There should be logic to sign into Azure and select the right tenant and subscription.
CC:// @1jc
|
||
#Check if storage account exists | ||
#Returns a CheckNameAvailabilityResult object, use property NameAvailable to get boolean value of if the name is available | ||
$doesntExist = Get-AzStorageAccountNameAvailability -Name $storageAccountName |
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.
@theanticrumpet I would prefer that you use ARM template to create the storage account instead doing this manually in the script. ARM templates provide additional benefits like for example checking if the resource exist, can be run asynchronously etc.
I would suggest you investigate that option.
CC:// @1jc
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.
Definitely looks like I've got more reading to do on general Azure + Powershell scripting instead of specific functions
Write-Host "Storage Account $storageAccountName already exists" | ||
} | ||
|
||
Read-Host -Prompt "Press enter to continue" |
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 line will block the script for executing without manual interaction. Not a good practice for automation scripts. CI/CD and other automation systems will not be able to use your script.
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.
Wait, do I want to take user input or not? Wouldn't the previous examples of requiring user input (EG: for resource group) also halt? Should I just make the whole thing take command line arguments as well?
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 might be wrong but for those dynamic variables you can use the Key Vault and share them within your team.
created files, updated index.html
NSC repo changes toddy into theKunte fork
Resolves #7