Added preliminary Rust binding to a whole lot of tspconfig.yaml files #38890
Conversation
Next Steps to MergeNext steps that must be taken to merge this PR:
Comment generated by summarize-checks workflow run. |
github-actions
bot
added
data-plane
resource-manager
RPaaS
TypeSpec
LarryOsterman
added
the
PublishToCustomers
github-actions
bot
added
BreakingChange-Python-Sdk
BreakingChange-JavaScript-Sdk
labels
LarryOsterman
requested review from
TimLovellSmith,
a-santamaria,
ankushbindlish2,
damodaravadhani,
dsouzaarun,
iliu816,
maririos,
mikeharder and
raych1
as code owners
…Osterman/azure-rest-api-specs into larryo/add_rust_tspconfigs
eng/tools/typespec-validation/src/rules/sdk-tspconfig-validation.ts
Outdated
Show resolved
Hide resolved
eng/tools/typespec-validation/src/rules/sdk-tspconfig-validation.ts
Outdated
Show resolved
Hide resolved
| flavor: "azure" | ||
| generate-test: true | ||
| generate-sample: true | ||
| "@azure-tools/typespec-rust": |
There was a problem hiding this comment.
Do all these SDK successfully generate currently? @raych1 are there any plans to have an SDK validation pipeline for rust?
@LarryOsterman @heaths we should probably avoid adding all these configurations until we have some sort of validation pipeline for them.
There was a problem hiding this comment.
are there any plans to have an SDK validation pipeline for rust?
I heard from @ronniegeraghty several months ago for the interest to onboard Rust to the SDK Validation pipeline. However, I haven't noticed any follow-ups yet.
@LarryOsterman @heaths do we want to onboard Rust to the SDK Validation pipeline?
There was a problem hiding this comment.
Right now, we can't onboard a rust pipeline because we have a ton of validation errors. A large part of the purpose f this PR is to make it simpler for the Rust SDK team to bulk compile the azure-rest-api-specs repo to find the gaps between spector and the real validators (gaps we've found for example: CBOR support, Text/Plain support).
Once we've done the validation, I think a validation pipeline is a great idea.
There was a problem hiding this comment.
We have individual pipeline to support the batch run on multiple specs. The config changes do not have to be in the main branch. The pipeline can be triggered against a feature branch.
| sub-rules: | ||
| - options.@azure-tools/typespec-rust.* | ||
| paths: | ||
| - storage/Microsoft.BlobStorage/tspconfig.yaml |
There was a problem hiding this comment.
Adding rust to the other specs with existing suppressions makes sense. But why do you need this new suppression, instead of adding the rust config to this spec, like you did the other specs?
There was a problem hiding this comment.
I couldn't find any supressions that were ONLY Microsoft.BlobStorage - I found convenient ones for the other suppressions. And the Go suppression above has a LOT of other packages beyond just the few which were failing
There was a problem hiding this comment.
Many of the existing suppressions in this file were added as temporary stop-gap. Most of the suppressions are grouped by service, but I see there was recently a group of suppressions added for "go". Before we add even more suppressions (debt) for rust, let's see how we can minimize the list.
There was a problem hiding this comment.
FWIW, as far as I could tell, the vast majority of these suppressions are for services which don't have a relevant language section in their tspconfig.yaml file.
These aren't about specific checks that need to be suppressed, they are for packages that don't have a @typespec-go or whatever section.
There was a problem hiding this comment.
If adding a @typespec-go section is opt-in, I don't think specs without the exception should require an exception. There should be a less-exceptional way to configure this. We should review this design before merging this PR.
weshaggard
left a comment
weshaggard
left a comment
There was a problem hiding this comment.
Let's not merge this PR until after the new year and we have onboarded the rust sdk validation pipeline. I want to ensure these all work before adding the configurations.
Co-authored-by: Mike Harder <[email protected]>
|
This PR doesn't need ARM review: adding ARMChangesRequested to get it out of the ARM reviewer's queue. |
github-actions
bot
removed
the
WaitForARMFeedback
6 participants
SDK configuration pull request
Purpose of this PR
Add Rust bindings to a significant number of tspconfig.yaml files. Note that these bindings do NOT imply that Rust clients will be generated for these packages, neither does it imply that the namespaces for these packages is correct.
Before the decision is made to generate a Rust SDK, please confirm with the architecture board that the package name is correct as is the working directory for the generated package.
Due diligence checklist
To merge this PR, you must go through the following checklist and confirm you understood
and followed the instructions by checking all the boxes:
tspconfig.yamltemplates:Getting help
Purpose of this PRandDue diligence checklist.write accessper aka.ms/azsdk/access#request-access-to-rest-api-or-sdk-repositoriesNext Steps to Mergecomment. It will appear within few minutes of submitting this PR and will continue to be up-to-date with current PR state.queuedstate, please add a comment with contents/azp run.This should result in a new comment denoting a
PR validation pipelinehas started and the checks should be updated after few minutes.