chaostoolkit-incubator / chaostoolkit-azure

Chaos Toolkit Extension for Azure
https://chaostoolkit.org/
Apache License 2.0
22 stars 28 forks source link

Support Azure China Cloud and other #71

Closed xpdable closed 4 years ago

xpdable commented 4 years ago

One possible solution for #70

xpdable commented 4 years ago

Generally I don't like these too many duplicated entries, code snippets and imports. Possible to wait @buderre back from vacation.

Lawouach commented 4 years ago

Indeed.

xpdable commented 4 years ago

@buderre @Lawouach Yep, that is what worth to talk. Field azure_cloud is indeed not secret. However, if we put it into configuration, we need some more refactoring as the method of

def auth(secrets: Secrets) -> ServicePrincipalCredentials: 
...

def __get_credentials(creds):
    credentials = ServicePrincipalCredentials(
        client_id=creds['azure_client_id'],
        secret=creds['azure_client_secret'],
        tenant=creds['azure_tenant_id'],
        cloud_environment=eval("msrestazure.azure_cloud.{}"
                               .format(creds['azure_cloud']))
    )
    return credentials

May need to accept configuration as well. And all of the invokes of auth(secrets) may need to updates :) Free to talk if we do more changes on this :P

buderre commented 4 years ago

I see now. The Service Principal needs them as credential data. Maybe I'm wrong and the cloud_environment is part of (un)secret credentials :) Maybe you just go on. +1 from me. Don't forget the commits to squash and make them one commit only.

xpdable commented 4 years ago

Thanks @buderre , @Lawouach Since we are all on the same page, shall we move on to next step to merge the PR?

Lawouach commented 4 years ago

Appreciate the discussion here :)

To review though, I would very much like a single squashed commit please.

xpdable commented 4 years ago

Squashed, and let's review again

buderre commented 4 years ago

Can you enhance the CHANGELOG file and write your changes in a single line?

xpdable commented 4 years ago

@Lawouach @buderre some changes on the client init, need more review of this. The big benefit that no need to change more duplicates when we need more change on client init. eval() is also change to the function __get_cloud_env_by_name to avoid any risk. Meanwhile, I need to manage sometime on implementing more test of above.

xpdable commented 4 years ago

@buderre @Lawouach As I moved some init_client function to __init__.py, I do sincerely request you review on this.