-
Notifications
You must be signed in to change notification settings - Fork 66
Conversation
…tk into feature/getting_started_scripts
README.md
Outdated
@@ -16,28 +16,32 @@ This toolkit is built on top of Azure Batch but does not require any Azure Batch | |||
- Ability to run _spark submit_ directly from your local machine's CLI | |||
|
|||
## Setup | |||
|
|||
1. Clone the repo |
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.
Are we comfortable enough to just pip install? Technically it should be the same thing, right??
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.
technically, yes. But I elected not to make that change here until our release on pip is "official". I can do it now if you'd prefer.
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.
it just seems so much easier now... even if the version shows up as '0.6.1b' for example...
"Do you want to use this virtual network? (y/n): ".format(virtual_network_name) | ||
deny_error = AccountSetupError("Virtual network already exists, not recreating.") | ||
unrecognized_input_error = AccountSetupError("Input not recognized.") | ||
prompt_for_confirmation(confirmation_prompt, deny_error, unrecognized_input_error) |
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.
Never used this before, but does this exit the method?
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.
it will raise an error if either the user declines, or if in 3 tries the input is not recognized. Otherwise this returns none and just continues.
filter="roleName eq '{}'".format(role_name) | ||
)) | ||
contributor_role = roles[0] | ||
for i in range(10): |
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.
What are you doing here? Is this trying to assign the roles on a retry loop?
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.
For some reason, the GraphRbacManagementClient's service_principal.create() method does not return an AzureOperationPoller to wait on (ala most of the other clients) nor does it block until the object is created server side. It just returns an object which may or may not have been created server side yet.
So I need to wait to give permissions here until the service principal is actually created.
account_setup.py
Outdated
subscription_client = SubscriptionClient(creds) | ||
tenant_ids = [tenant.id for tenant in subscription_client.tenants.list()] | ||
if len(tenant_ids) != 1: | ||
raise AccountSetupError("More than one tenant configured on your subscription.") |
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.
Is there nothing the user can do here? I'm not even sure what this error means myself...
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 means that you're subscription has more than one tenant configured (don't know myself why that is possible/desired). I'm not sure what should be done here, or if we should bother. This seems like an extremely advanced case.
"resource_group": prompt_with_default("Resource Group Name", DefaultSettings.resource_group), | ||
"storage_account": prompt_with_default("Storage Account Name", DefaultSettings.storage_account), | ||
"batch_account": prompt_with_default("Batch Account Name", DefaultSettings.batch_account), | ||
# "virtual_network_name": prompt_with_default("Virtual Network Name", DefaultSettings.virtual_network_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.
un-comment?
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.
maybe this could be a 'leave blank for no vnet'
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 left this commented since we don't (currently) have a way to output the subnet_id since it is in cluster.yaml and everything else is in secrets.yaml.
account_setup.py
Outdated
parameters={ | ||
'location': kwargs.get("region", DefaultSettings.region), | ||
} | ||
) |
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.
Will this fail w/ a good error msg is the user provides bad inputs? E.g. region == westuss
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.
Right now this will just throw whatever error the ResourceManagementClient throws. I think we should probably catch this error, print a link to docs on azure regions, do a 3 attempt retry loop.
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 is the current error:
Traceback (most recent call last):
File "account_setup.py", line 336, in <module>
resource_group_id = create_resource_group(creds, subscription_id, **kwargs)
File "account_setup.py", line 79, in create_resource_group
'location': kwargs.get("region", DefaultSettings.region),
File "/home/jake/.local/lib/python3.5/site-packages/azure/mgmt/resource/resources/v2017_05_10/operations/resource_groups_operations.
py", line 147, in create_or_update
raise exp
msrestazure.azure_exceptions.CloudError: Azure Error: LocationNotAvailableForResourceGroup
Message: The provided location 'westuss' is not available for resource group. List of available regions is 'centralus,eastasia,southea
stasia,eastus,eastus2,westus,westus2,northcentralus,southcentralus,westcentralus,northeurope,westeurope,japaneast,japanwest,brazilsout
h,australiasoutheast,australiaeast,westindia,southindia,centralindia,canadacentral,canadaeast,uksouth,ukwest,koreacentral,koreasouth,f
rancecentral,eastus2euap,centraluseuap'.
|
||
The script will create and configure the following resources: | ||
- Resource group | ||
- Storage account |
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.
Should we add a vnet?
Virtual Network (optional)
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.
Possibly, but I'm not sure how we'd do output. We could potentially just leave this out until we have "environments"
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 still feel like we should use pip install and cut down on that step. It's probably not required any more. At that point though, might be worth moving these docs somewhere for users to want the 'latest and greatest' version.
"application_credential_name": prompt_with_default("Active Directory Application Credential Name", DefaultSettings.resource_group), | ||
"service_principal": prompt_with_default("Service Principal Name", DefaultSettings.service_principal) | ||
} | ||
print("Creating the Azure resources.") |
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 just went through this again and parts of this can take a while, especially getting the service principal. I would recommend adding more print statements in this piece of code make it feel like progress is being made.
) | ||
application_credential = uuid.uuid4() | ||
try: | ||
display_name = kwargs.get("application_name", DefaultSettings.application_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.
account_setup.py
Outdated
} | ||
) | ||
|
||
print("\n# Copy the following into your .aztk/secrets.yaml file\n", secrets) |
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 is still printing with a space. I believe this will work
print("\n# Copy the following into your .aztk/secrets.yaml file\n{}".format(secrets))
Fix #354