Azure / typespec-azure

About TypeSpec Azure Libraries
https://azure.github.io/typespec-azure/
MIT License
11 stars 37 forks source link

[Feature Request] Get ARM Server with hostParameters #217

Closed XiaofeiCao closed 2 months ago

XiaofeiCao commented 7 months ago

For ARM tsp, getServers from typespec/http will return a Server with no hostParameters. This is due to a synthesized Server generated by @armProviderNamespace: https://github.com/Azure/typespec-azure/blob/8b8d7c05f168d9305a09691c4fedcb88f4a57652/packages/typespec-azure-resource-manager/src/namespace.ts#L121-L127

This is not enough for SDKs. Since we allow customers to set host, e.g. Java:

private ApplicationInsightsManager(HttpPipeline httpPipeline, AzureProfile profile, Duration defaultPollInterval) {
        Objects.requireNonNull(httpPipeline, "'httpPipeline' cannot be null.");
        Objects.requireNonNull(profile, "'profile' cannot be null.");
        this.clientObject =
            new ApplicationInsightsManagementClientBuilder()
                .pipeline(httpPipeline)
                .endpoint(profile.getEnvironment().getResourceManagerEndpoint()) // set host
                .subscriptionId(profile.getSubscriptionId())
                .defaultPollInterval(defaultPollInterval)
                .buildClient();
    }

we need a Server parameter with hostParameters. Previously this is done by modelerfour(I guess).

Proposal:

We do either:

  1. Provide a function getArmServer in TCGC to get the Server for ARM. When dealing with ARM, use getArmServer instead of current getServers, as ARM will probably have one server only.
  2. Each language SDK handles on their own, e.g. Java PR
  3. Change the @armProviderNamespace synthesized Server to have hostParameters. Though this will probably affect typespec-autorest, since it doesn't need hostParameters.
  4. Implement it in getAllOperations in TCGC? (this I'm not sure)

I'm fine with both 1 and 2. There needs more discussion for 3 or 4.

haolingdong-msft commented 2 months ago

@tadelesh Current TCGC's response of endpoint for ARM client is like below: image It will write "https://management.azure.com" as serverUrl which may not be correct because we allow user to set host. So I guess it should be "{endpopint}"?

archerzz commented 2 months ago

I think it's a language specific issue: service team define constant value in the @server, and it's Java (.net as well) which decides to let SDK users to overwrite anyway. So I believe the change should be in the language emitter/generator.

tadelesh commented 2 months ago

i vote for 3 since arm has different entry point. @armProviderNamespace should not hard code host value.

haolingdong-msft commented 2 months ago

I also vote for 3, but it might impact the generated swagger, so we will need more input from TypeSpec ARM team.

tadelesh commented 2 months ago

final decision: change arm decorator.

haolingdong-msft commented 2 months ago

Hi @allenjzhang, may I know the plan for this issue? What is the ETA of the task?

timotheeguerin commented 2 months ago

It feels to me like changing the host for client should be something that is always available regardless of what is configured to test against localhost for example

haolingdong-msft commented 2 months ago

@timotheeguerin I agree we should be able to change the host for this ARM case, but not sure if it's always changable, as for unbranding, user can only allow a specific endpoint, but not able to change. The problem is for ARM case, current TypeSpec definition does not allow changing the host. So we would need TypeSpec change to allow host to be changable with a default value. Current definition only allow one value.

allenjzhang commented 2 months ago

The host should always be settable by the client. For example, ARM also has different sovereign cloud and azure stack endpoints which client needs to specify. Making ARM spec have parameterized host seems overkill and not necessary.

haolingdong-msft commented 2 months ago

I think we need to make ARM spec have parameterized host, otherwise it's not changeable. If we always treat the host as changable even with constant host url, the problem is non-azure may have needs to not change the host.

timotheeguerin commented 2 months ago

So if someone wants to test with a test url(localhost or staging server) they have to make a different spec? This feels like a terrible experience, why not always allow it.

haolingdong-msft commented 2 months ago

Hi @timotheeguerin, I don't have strong opinion on how to treat constant endpoint defined in TypeSpec. I'm fine with always treating the host as settable even it's a constant, as long as we have consistent behavior across Azure and non-Azure.

We previously discussed with @srnagar and agreed we have scenarios that the host cannot be changed. Chenjie and I have a discussion with Allen just now, Allen will talk with SDK archs again and have agreement on how to treat constant host. @allenjzhang Please update if you have agreement, Thanks!

iscai-msft commented 2 months ago

The decision following our sync was to first implement this in TCGC, with later consideration of moving this into the compiler. TCGC will special case host parameters to make them a string with a clientDefaultValue. Here is the issue to revisit it in the typespec compiler: https://github.com/microsoft/typespec/issues/3821

timotheeguerin commented 2 months ago

Just to clarify here too but it should be the baseUrl that is overridable not the host(Basically the entire thing that @server would set. Without that you are blocking many scenarios still:

allenjzhang commented 2 months ago

Per discussion in DPG sync meeting, we have discussed that:

  1. Client with overridable host is fairly universal.
  2. Having it centralized in TCGC to indicate the host is writeable is a good choice.

There was unanimous agreement in DPG sync meeting today. Issy opened issue to track the discussion on TypeSpec compiler side. https://github.com/microsoft/typespec/issues/3821

timotheeguerin commented 2 months ago

Filed a dedicated issue for tcgc https://github.com/Azure/typespec-azure/issues/1153