This will be removed before merging to master but needed currently as SDK not released.

This will be removed before merging to master but needed currently as SDK not released.

Some example API profiles as non have been published yet.

@lmazuel naming okay here?

@derekbekoe so far so good :)

Place the class member field in front of any methods.

@troydai done.

Why not just have a separate argument for resource_type with a default of None? client_type_or_resource_type is not a great name (at least shorten to client_or_resource_type?)

Why not just have a separate argument for resource_type with a default of None?

One argument is required so adding another argument resource_type=None would not work as client_type would still be required.

I've shortened the name to client_or_resource_type.

Does that work?

I would recommend shortening this to "api_version", especially if it's going to be used all over the place (which I suspect it will)

I'm all for a shorter name. However, a function name better has a verb in it.

A lot of our other methods start with a verb so tempted to stick with the names as they are.

Also, a typical use-case of the method is api_version = get_api_version(resource_type). If I renamed the method, api_version = api_version(resource_type) would cause issues as the method name equals the variable name and api_version would be a very common variable name here.

I'm fine with that, but we need to abstract out the API version as much as possible so we don't have "get_api_version(...)" all over the place.

Same here--recommend dropping the "get_"

Also, please add doc comments so we know what "model" and "checked" do. If model is left as None does it return ALL models?

Also, please add doc comments so we know what "model" and "checked" do. If model is left as None does it return ALL models?

Done.

I left the method name the same... see my comment above.

Instead of a pass, couldn't we spell out the error message here?

if you pass, this means you get the same stacktrace but typed as APIVersionException (you get the init from your father). If we have no info to add (like an api_version or something) this is ok for me.

Fixed by overriding Exception to define my own __str__ error message here.

So all of the profiles are statically hard-coded in the CLI? What is the plan to ingest some kind of JSON document that contains this information? I would expect a profiles directory that contains a JSON document for each supported profile (except maybe latest).

If we are decoupled to the point that we can change this implementation without breaking our "get_api_version" and "get_versioned_models" interface then this is fine for now.

I totally agree with you @tjprescott , as long as we decouple enough I get the hardcode for now.

So all of the profiles are statically hard-coded in the CLI?

It would go in a shared package that's used by both the CLI and the Python SDK.

Yes it's decoupled. Everything in the shared.py file will eventually be moved out to a shared package to be used by both the CLI and SDK.

Here again, the original is clearly superior to the new. Why not something like:

Encryption, EncryptionServices, EncryptionService = versioned_models(ResourceType.MGMT_STORAGE_STORAGE_ACCOUNTS, 'Encryption', 'EncryptionServices', 'EncryptionService')

This isn't great, but it's closer to the "easy" way and is easily implemented with minor changes. In reality, I would prefer it look like:

Encryption, EncryptionServices, EncryptionService = versioned_models(ResourceType.MGMT_STORAGE_STORAGE_ACCOUNTS)

So that it is, for all intents and purposes, functionally identical to the original while still handling the version stuff, but a solution does not immediately spring to mind for how to accomplish this--however, doing this would solve the ugly syntax of importing the enum for the choice list as well.

Gone with option 1 here.

Option 1 is fine for now, but I think before we can really move forward with wholesale implementation, we will need something like option 2.

@tjprescott How would Option 2 be achieved technically?

What is this?

The commands that only apply to certain API versions go in this file.

Does this API call not exist in previous versions or is this an example only? This illustrates why I would prefer "api_version" over "get_api_version", but I would actually prefer something that looked like:

cli_generic_update_command(__name__, 'storage account update', 
   mgmt_path + 'get_properties', 
   mgmt_path + 'update', factory, 
   custom_function_op=custom_nonce_path + 'update_storage_account',
   min_api_version='foo', max_api_version='bar') 

Where min and max have defaults. The commands.py files are (in most modules) very straightforward and easy to read, and this preserves that. Adding conditionals makes them more difficult to maintain. Also, by putting this as part of the command, you can see the supported API versions in the command_table metadata. With if statements, you will not have that information.

Will look into this

I would also suggest we could make this a follow up issue after the PR is merged.

I'll leave this as is for now.
With the min_api_version='foo', max_api_version='bar' suggestion, it doesn't include which resource types the api versions correspond to. Currently, basic if statement logic gives the most flexibility. As I implement this for other modules, a pattern should appear then we can build more support in the CLI as described.

This is another example of this syntax not being workable for practical purposes. Everyone is just going to put a string here...

This already looks like a maintenance headache. The differences here are not so profound as to duplicate the basic logic. Only the initialization of StorageAccountCreateParameters is different.

We should investigate having a way of defining and easily getting method signatures for different api versions. Something like:

storage_account_create_sig = {
  'profile1': (resource_group_name, account_name, location, account_type, tags=None),
  'profile2': (resource_group_name, account_name, location, sku, kind, encryption=None, access_tier=None, tags=None)
}

Then you have a way of getting an API specific signature for reflecting on parameters and so forth and you can call into an implementation method by treating those signatures as kwargs.

Will look into this.

I would suggest this will be a tricket wicket to solve. I'd be fine with merging the PR, creating issues for the rough spots, and then, once those issues are fixed, moving forward with the wholesale implementation.

I'm not a huge fan of the multiple return type here (Single Responsability Principle). I would prefer something like:

def get_versioned_models_module(api_profile, resource_type, checked=True):
    api_version = get_api_version(api_profile, resource_type)
    try:
        return get_client_class(resource_type).models(api_version=api_version)
........

def get_versioned_model(api_profile, resource_type, model, checked=True):
     try: 
        return getattr(get_versioned_models_module(api_profile, resource_type), model)
     excpet........

I have removed the ability to get the full module. So now only get_versioned_model will exist.

The namespace is contradicting itself. If it is shared, it shouldn't be a private namespace.

The shared package will eventually be moved out of the CLI and into a shared package that the CLI and SDK will share.
Changed to remove the underscore for now though.

Add an example in the comments to improve readability.

done.

I'm all for a shorter name. However, a function name better has a verb in it.

Place the class member field in front of any methods.

I second what @tjprescott said. The pattern is way to confusing and unintuitive.

Why lambda here?

@troydai I could have done this inline as was the case before but since this method is slightly longer, I assigned the lambda to a variable.

I'm happy to move it to a normal method or back to inline. Thoughts?

PEP8:
Always use a def statement instead of an assignment statement that binds a lambda expression directly to an identifier.

Yes: def f(x): return 2x
No: f = lambda x: 2x

Also, getattr has a default value parameter, so your lambda is the same as:

getattr(x, 'keys', x)

yes thanks

To be honest, this doesn't look good. What if there future API breaks? The control flow will look more and more complex. Should there be different custom command mapped to different version of API modules?

This is the only line that's different and changes like this will not happen often.
Not sure if duplicating the whole method for this one change is the right way to go.

I agree with @derekbekoe. I would rather the API difference be called out explicitly rather than implies through try/except magic, but duplicating a method just duplicates your unfound bugs.

I agree to do not duplicate. However, maybe you can try/except keys = [obj.key1, obj.key2] as well with a custom message like "unexpected format" or something. Otherwise here you will get an AtributeError on "key1" if something wrong in one year. Not critical however.

The shared package will eventually be moved out of the CLI and into a shared package that the CLI and SDK will share.
Changed to remove the underscore for now though.

done.

Why not just have a separate argument for resource_type with a default of None?

One argument is required so adding another argument resource_type=None would not work as client_type would still be required.

I've shortened the name to client_or_resource_type.

Does that work?

A lot of our other methods start with a verb so tempted to stick with the names as they are.

Also, a typical use-case of the method is api_version = get_api_version(resource_type). If I renamed the method, api_version = api_version(resource_type) would cause issues as the method name equals the variable name and api_version would be a very common variable name here.

Also, please add doc comments so we know what "model" and "checked" do. If model is left as None does it return ALL models?

Done.

I left the method name the same... see my comment above.

The commands that only apply to certain API versions go in this file.

Will look into this.

Will look into this

This is the only line that's different and changes like this will not happen often.
Not sure if duplicating the whole method for this one change is the right way to go.

Will look into this.