Azure / azure-sdk-tools

Tools repository leveraged by the Azure SDK team.
MIT License
113 stars 177 forks source link

[tsp-client] tsp-client fails in SDK automation running on azure-rest-api-specs-pr #8012

Closed weidongxu-microsoft closed 6 months ago

weidongxu-microsoft commented 6 months ago

The tsp-client used in that time is 0.5.0 (SDK automation would use latest -- at the time it happens, latest is 0.5.0)

Error message

Error: Cloning into '/mnt/vss/_work/1/s/azure-sdk-for-java-pr/../sparse-spec'...
fatal: could not read Username for 'https://github.com'/: No such device or address

Details: https://github.com/Azure/azure-sdk-for-java/pull/38983#discussion_r1506942699

lirenhe commented 6 months ago

@catalinaperalta, could you take a look?

cc @lmazuel

raych1 commented 6 months ago

@catalinaperalta , please note that this issue blocks the tsp-client adoption for all SDK language automation tool. See point#4 in this epic for details.

catalinaperalta commented 6 months ago

@weshaggard what are your thoughts on the ideas I posted in this comment. Copying the same comment below for easy access:

IIRC in the pipeline we'd need to have git credentials configured for the git sparse checkout command to work. A few months ago I spoke to Ben about this since I'd seen this issue https://github.com/Azure/azure-sdk-tools/issues/3222 opened about this exact scenario. I think we'd need to have a PAT or git credentials configured in the pipeline for this to work. Locally for private spec repo support we rely on folks already being logged in with git/having their credentials configured. If we want to use a PAT in the pipeline...then I think there would be additional work to do in tsp-client. Otherwise if we can configure credentials in the pipeline like it's mentioned in https://github.com/Azure/azure-sdk-tools/issues/3222#issuecomment-1111116452, I believe tsp-client should work as is.

catalinaperalta commented 6 months ago

Thoughts on which route would be best to use in our pipelines? PAT or configuring git credentials as suggested here ? @weshaggard have we already enabled the persistCredentials config in the pipeline?

After we decide on the best method to enable git authentication in the pipeline, we can see if there are any updates required in tsp-client.

weshaggard commented 6 months ago

We have not test presistCredentials yet but presumably it would work. If not we would need to use a gh PAT.

@weidongxu-microsoft or @raych1 do you know how the current automation pulls files from the private repo?

edit: @catalinaperalta I might be mistaken but I believe the current process works because the repo is already cloned locally in the automation. In which case does tsp-client support working against an already closed repo?

edit: Actually looking again at the error it seems we are talking about the private java repo. So my question still stands @weidongxu-microsoft how do you currently clone that?

catalinaperalta commented 6 months ago

Yes we support a --local-spec-repo flag so the automation script could do something like:

tsp-client update --local-spec-repo <path to local spec>

Example using an actual spec on my machine:

tsp-client update --local-spec-repo ..\..\..\..\azure-rest-api-specs\specification\ai\DocumentIntelligence\
catalinaperalta commented 6 months ago

If the repo isnt cloned and we need to use a PAT then I would need to add that functionality in tsp-client FYI.

weshaggard commented 6 months ago

@raych1 I'm also curious about another aspect which is the creation of the tsp-location file that https://github.com/Azure/azure-sdk-tools/blob/main/eng/common/scripts/TypeSpec-Project-Process.ps1 does I'm not sure if that aspect will be done by tcp-client. Correct me if I'm wrong @catalinaperalta.

catalinaperalta commented 6 months ago

tsp-client does create the tsp-location.yaml file with the init command

weshaggard commented 6 months ago

Looking at the logs of a failing run at https://dev.azure.com/azure-sdk/internal/_build/results?buildId=3520093&view=logs&j=a8a7a537-82b0-583c-7971-bac70b9822ca&s=eb9754a7-3885-5b5b-bd91-16c95dd7881e&t=37e3947b-3cfb-5d36-86ba-0e22bb7dbc33&l=25 the private java repo is already being cloned. So I guess the next question is @catalinaperalta is there an option to also pass in the path to the local clone of the language repo as well?

catalinaperalta commented 6 months ago

We dont clone the SDK repos in tsp-client. We simply work within the repo that the tool is run from (in other words we're "unaware" of what repo we're running in as long as it has the right config which is having an eng/ directory with an emitter-package.json file). I thought we were failing on cloning the private specs repo?

weshaggard commented 6 months ago

The error message might be a little mis-leading. Error: Cloning into '/mnt/vss/_work/1/s/azure-sdk-for-java-pr/../sparse-spec'...

Looks like it is trying to clone the specs repo into a relative path under the java-pr repo. However, from the logs it seems like the private specs repo is also cloned. So perhaps the missing piece here is that local cloned specs repo needs to be passed to tsp-client. @weidongxu-microsoft so this should be supported if you pass in that location to tsp-client.

weidongxu-microsoft commented 6 months ago

@catalinaperalta

Does tsp-client init also support the --local-spec-repo option? If yes, I can try that.

catalinaperalta commented 6 months ago

@weidongxu-microsoft can you give me some more information on what the goal of the automation script is? I'd like to understand what we want the pipeline to do, so we can get to the right tsp-client command. :)

weidongxu-microsoft commented 6 months ago

I (maybe dev from other languages as well) want the tool to do 2 things.

  1. create the "tsp-location.yaml" file at the correct place (that require parsing the "tspconfig.yaml", and figure out the target folder in SDK repo for the module)
  2. basically run tsp-client update in that target folder, as now we had the "tsp-location.yaml"

I'd assume current tsp-client init do the above 2 things. But I think it would pull the remote repo in step 2 (if not specificly configured).

On step 2, I can see you have a --local-spec-repo that can point to a local spec repo, which I think Ray have in the SDK automation env. <-- one quick question on this, if --local-spec-repo is provided, does tsp-client ignore the commit and repo in "tsp-location.yaml" file?

And I'd assume step 1 does not need to pull anything remote (it only need the "tspconfig.yaml")

That is the reason I am asking whether e.g. tsp init --local-spec-repo is supported, that it can do step 1 and step 2 without pulling the remote.

@raych1 Correct me if I am wrong somewhere.

weidongxu-microsoft commented 6 months ago

I tried

tsp-client init --tsp-config /c/github/azure-rest-api-specs/specification/ai/OpenAI.Assistants/tspconfig.yaml --commit 106483d9f698ac3b6c0d481ab0c5fab14152e21f --repo Azure/azure-rest-api-specs --local-spec-repo /c/github/azure-rest-api-specs

It works if I have network, but it give Error: Cloning into 'C:/github/azure-sdk-for-java/../sparse-spec'... if I turn network of my PC off.

raych1 commented 6 months ago

@catalinaperalta , the automation scripts are chained together to support the SDK generation, which is based on the specification in the spec pull request. The CI automation script clones both the spec repo and the SDK language repo to the local AzureDevOps agent. These repos then can be passed to tsp-client to run the emitter.

catalinaperalta commented 6 months ago

Added support for local specs with the init command via #8121. Triggering the release pipeline now, so the updated package verison should be available soon. EDIT: update is released please update to version 0.7.0 of the package to get the changes. @weidongxu-microsoft you should now be able to use tsp-client init with the cloned version of the specs repo in the pipeline. Here is an example command:

tsp-client init --tsp-config ..\..\..\azure-rest-api-specs\specification\cognitiveservices\ContentSafety\tspconfig.yaml --commit aaf40b00d0d15db7d3252beeeac634da9dc09099 --repo Azure/azure-rest-api-specs --local-spec-repo ..\..\..\azure-rest-api-specs\specification\cognitiveservices\ContentSafety\ 

If you run into any other issues feel free to ping me/open a new issue.

weidongxu-microsoft commented 6 months ago

Thanks Catalina. Tested 0.7.0 on local, it works as expected.

Would do a PR to put it to automation. https://github.com/Azure/azure-sdk-for-java/pull/39829