Azure / azure-rest-api-specs

The source for REST API specifications for Microsoft Azure.
MIT License
2.6k stars 5.01k forks source link

Service Fabric Spec is Not ARM Spec #207

Closed devigned closed 8 years ago

devigned commented 8 years ago

The host for the service fabric spec points to localhost and clearly couldn't be used in it's current form for code generation or external use. It does not belong in the master branch until it is done.

@msjeffreychen, please move this to a branch or request that @amarzavery move this to a branch.

@msjeffreychen, please rename the folder from 'arm_servicefabric' to 'servicefabric'.

amarzavery commented 8 years ago

@devigned - I had brought up the same point with jefferey Chen in the initial review of the spec. He said that is the right value for the host. It is not a regular arm service. It is a hosted service. Since he clarified it with his PM about this, I was fine with it. They are using the generic version of nodejs codegen.

devigned commented 8 years ago

@amarzavery I see. That's quite interesting. This is one of those places where I wish we stored the specs as yaml, so that we could add comments to the spec and describe why some things are the way they are.

Though, I don't think the spec should be in the 'arm_' folders. The spec is not an ARM API spec, it is a data plane spec for service fabric.

I'm going to change the title and redirect the issues.

amarzavery commented 8 years ago

@devigned - I argued the same thing. He published a node package named "azure-servicefabric" and then published another package named "azure-arm-servicefabric" insisting that his service is an ARM service.

amarzavery commented 8 years ago

Highlighting the important point from the review in the PR #129 "Our commands will read a local config file to determine the hostname, we are not really using the hostname in the swagger file."

devigned commented 8 years ago

@amarzavery ok, then there should be a arm hostname in the spec and the commands can override that in the presence of the local config.

msjeffreychen commented 8 years ago

I looped in our PMs to explain in more detail. Thanks for bringing this up. I have checked again with our management. The story is that our team owns both SFRP (Service Fabric Resource Provider), and the software Service Fabric. SFRP is indeed an ARM service. Users could interact with SFRP to create Service Fabric clusters. The CLI commands and spec we added so far are used to interact with Service Fabric clusters, not SFRP. In this case, how should we name our spec? If we also added the spec to interact with SFRP in the future, would that confuse the customer, as some commands are under ARM, some commands are not? Do you have other customers having the similar situation? How should we spec this? Thanks.

devigned commented 8 years ago

@msjeffreychen, we expect there will be both an RP api and a service api (we call that management plane vs. data plane). We expect the management plane to be /arm-{service name}/ and the data plane to be /{service name}/. For the library naming, we suggest azure-arm-{service name} for management and azure-{service name} for data plane. A good example of this is azure-arm-storage and azure-storage.

If we all follow the same guidance, then there will be consistency and less likelihood of customers getting confused.

msjeffreychen commented 8 years ago

I will rename them. But, the CLI only supports 2 modes, ARM and ASM. In this case, how should I place my commands?

amarzavery commented 8 years ago

There are some commands that are mode agnostic. For example: azure login command. Anything that is common to both the modes or does not care about the mode should go here. Please take a look at the storage and batch commands.

devigned commented 8 years ago

I believe this can be closed since the spec was moved to an appropriate folder.