Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove the hard-code for patch LRO #2583

Merged
merged 23 commits into from
Jul 16, 2024
Merged

Conversation

MaryGao
Copy link
Member

@MaryGao MaryGao commented Jun 14, 2024

fixes #2314

Previously there is some issues with getLroMetadata for PATCH method from TypeSpec. In the latest compiler version these issues are resolved and we need to remove these hard-codes.

@MaryGao MaryGao changed the title Remove the hard-code for patch LRO [Blocked]Remove the hard-code for patch LRO Jun 17, 2024
return context
.path("/test-runs/{testRunId}:stop", testRunId)
.path("/test-runs/{testRunId}:stopTestRun", testRunId)
Copy link
Member Author

@MaryGao MaryGao Jun 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

issue tracked here: Azure/typespec-azure#1021
The referred path in RLC and modular is different and we could reproduce this in playground.

It should be a bug in TCGC or http lib.

  • RLC
interface Foo {
  @doc("Stop test run by test run Id.")
  @operationId("LoadTestRun_StopTestRun")
  stop is StandardResourceOperations.ResourceAction<FileInfo, {}, FileInfo, {}>;
}

The path we got is /files/{fileName}:stop.

  • Modular
@client({
  name: "LoadTestRunClient",
  service: Contoso.WidgetManager,
})
interface TestRunOperations {
  stopTestRun is Foo.stop;
}

The path we got is /files/{fileName}:stopTestRun.

@MaryGao MaryGao changed the title [Blocked]Remove the hard-code for patch LRO Remove the hard-code for patch LRO Jun 17, 2024
@MaryGao MaryGao removed the blocked label Jun 28, 2024
@MaryGao MaryGao marked this pull request as ready for review July 9, 2024 04:05
options: ListMetricDimensionValuesOptionalParams = { requestOptions: {} },
): PagedAsyncIterableIterator<DimensionValueList> {
): Promise<DimensionValueList> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is changing from pagable to non-pagable a breaking? Also why would they have this change, it should be common to see changing from non-pagable to pagable, but should be uncommon in the reverse case?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@weidongxu-microsoft Not sure you have more context. In the latest loadtesting spec,

  • there is no LRO now
  • listMetricDimensionValues is changed from ResourceList to Operation but becoming non-paging

Copy link
Member

@weidongxu-microsoft weidongxu-microsoft Jul 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The LRO in Java is handwritten
https://github.com/Azure/azure-sdk-for-java/blob/main/sdk/loadtesting/azure-developer-loadtesting/src/main/java/com/azure/developer/loadtesting/LoadTestRunAsyncClient.java#L127-L146
from simple API like createOrUpdateTestRun getTestRun.

(from what I remember, the status in GET response etc. does not match Azure guideline and SDK core lib handling of LRO -- that was the api-version in main, this version in PR maybe different)

Copy link
Member Author

@MaryGao MaryGao Jul 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The LRO change is expected and the paging change is waiting for the service team's reply, but I think we don't need to block this pr since this is service spec change. Let me offline confirm this.

@MaryGao MaryGao merged commit 8057e18 into Azure:main Jul 16, 2024
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support patch lro in modular
4 participants