sync the whole resource provider directory in azure-sdk-for-net #5807
Conversation
chunyu3
requested review from
konrad-jamrozik,
weshaggard and
benbp
as code owners
|
The following pipelines have been queued for testing: |
| @@ -93,7 +97,15 @@ $configuration = Get-Content -Path $typespecConfigurationFile -Raw | ConvertFrom | |||
| $pieces = $typespecConfigurationFile.Path.Replace("\","/").Split("/") | |||
| $projectName = $pieces[$pieces.Count - 2] | |||
|
|
|||
| $specSubDirectory = $configuration["directory"] | |||
| # clone the whole RP directory which is the parent of $configuration["directory"] | |||
| if ($configuration["directory"] -match "^[^/\\]+[\\/]+[^/\\]+") | |||
There was a problem hiding this comment.
What kind of pattern is this supposed to match?
There was a problem hiding this comment.
Should we just walk up the parents until we hit the "specification" folder?
There was a problem hiding this comment.
Yes. It will get the top-level directory under specification.
There was a problem hiding this comment.
This regex doesn't say to me that it is looking for specification folder. What do we expect the values of $configuraion["directory"] to be? Are they relative paths from the specification folder? or from the root of the repo? If they are from the root I would expect this regex to have the work "specification" in it somewhere.
| Write-Host "Copying spec from $source to $dest" | ||
| # $mainSpecDir is the PR folder, we just need to copy its subfolders which include the typespec project folder | ||
| Get-ChildItem –Path "$source" -Exclude @("data-plane", "resource-manager")| |
There was a problem hiding this comment.
I think we can also sync these folders there isn't any real value in excluding them.
There was a problem hiding this comment.
Hello @weshaggard data-plane and resource-manager is useless for generating SDK from typespec, and it is huge, so we skip to copy them to our template folder.
There was a problem hiding this comment.
I know they have specs but are they really that big? Do we have any real data that says they make it take too much time? It just feels like if we can eliminate this special case it might make things easier.
There was a problem hiding this comment.
If we decide to exclude them after all, I think adding a comment explaining why is that would be worthwhile. Would be nice if it would say how big is the slowdown.
| Get-ChildItem –Path "$source" -Exclude @("data-plane", "resource-manager")| | ||
| Foreach-Object { | ||
| Copy-Item -Path $_.FullName -Destination $dest -Recurse -Force | ||
| } | ||
|
|
||
| foreach ($additionalDir in $specAdditionalSubDirectories) { |
There was a problem hiding this comment.
Now that we are copying the entire rp folder do we even need additional directories any longer?
There was a problem hiding this comment.
Yes. We will copying entire rp folder. The dependency usually be defined via import *** in a .tsp file, now we have no way to identify what directories are needed for this typespec project, so the possible solution (maybe workaround now) is to sync down the entire rp folder.
@m-nash Can you give me more data on how long it takes to get the entire spec folder vs just the one TypeSpec folder? The sparse-checkout scoping to the one spec folder seems like it should be pretty quick. Do you have an example of what you are worried about? One of the issues we are trying to fix here is that new services that have shared libraries don't have a place to provide those additionalDirectories purely in the Spec PR (for example Azure/azure-rest-api-specs#23168). We want to enable those new specs to be able to generate without needing to commit changes into the sdk repo to make that happen. |
|
@m-nash and I chatted more about this and agreed that we don't think we need to support syncing the whole directory and using AdditionalDirectories should still work fine. However in order for that to work for new projects like my sample we need a couple of changes to how things are working today. First we need a way to specify those AdditionalDirectories in the tspconfig.yaml file in the specs repo similar to other package settings. Then we need to have the sdk automation in the specs repo start to generate a tsp-location.yaml file with this and the other sha information in it and pass that file to the language repository scripts that do the generation. We also need to add support for passing in the path to the already cloned specs repo to those language generation scripts as we shouldn't need to pull those files again since we already have the specs repo cloned. @raych1 can you please help work on this approach? |
Hello @weshaggard will the
Thanks |
The second one. This will only be consume in the case where we are generating from the spec repo and the project does not exist at all. Otherwise we wouldn't be creating a tsp-location but there could be an edge case where the additional directories change between spec PRs. |
|
Yes I agree this would only be done from the specs repo (or in cases where we are starting from the spec repo). In the case of the spec change we would always need to create/update the tsp-location.yaml file with any new changes to AdditionalDirectories as well as update to the new sha which the spec is currently based on. |
|
It sounds good to me. I created above linked issue for tracking. I talked to other SDK owners and they would turn to use common scripts for sdk generation once they're ready. |
|
Nice 👍 |
|
Can we close this PR since additionalDirectories will now be handled outside of this script? Or can we update the description to reflect the new approach if there is one? |
Fix #5808