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

Update ShareLink sample #21598

Merged
merged 2 commits into from
Jun 5, 2021
Merged

Update ShareLink sample #21598

merged 2 commits into from
Jun 5, 2021

Conversation

heaths
Copy link
Member

@heaths heaths commented Jun 4, 2021

Fixes #21543

@heaths heaths requested a review from schaabs as a code owner June 4, 2021 19:01
@ghost ghost added the KeyVault label Jun 4, 2021
@heaths
Copy link
Member Author

heaths commented Jun 4, 2021

I'm pondering just splatting all the files in this sample we need and keep it as a static copy to avoid updates breaking the sample. An updated sample went out that doesn't build when downloaded from Samples Browser because of some refactoring. Some files weren't added to the autorest.csharp package, which I also had an older version for in the .csproj to make sure it builds.

This should fix the problem for now, but we may need to think longer-term about samples.

That said, this sample is unique: it's actually relying on autorest.csharp to generate code for a feature of the service we don't support in track 2, so it does need autorest.

@christothes
Copy link
Member

Is there a reason why we couldn't add this sln to the paths that get build in CI?

@pakrym
Copy link
Contributor

pakrym commented Jun 4, 2021

Yeah, we (Heath) have been fixing this sample way too many times by now. We need something to run it in CI.

@heaths
Copy link
Member Author

heaths commented Jun 4, 2021

@christothes it does actually build when sdk/keyvault is built (or the core pipeline). The problem is that it has all the advantages of the various eng/* targets and props, which it doesn't when built from the Samples Browser. It actually can build in three modes. I added a CopySource target which I originally did to try to simulate Samples Browser ingestion a little. It's not exactly the same, but does move all the source out of our repo (depending on your /p:Destination value) so I can build and see if it works successfully. It's even called out in the README.

I do think we need to continue discussing how to do samples easily, reliably, and with some benefits (e.g. build and code analyzers to make sure our samples are good and consistent), but without inheriting all the other free stuff that won't work outside the repo.

@heaths
Copy link
Member Author

heaths commented Jun 4, 2021

I got to thinking that if I delete the Shared\Core and Shared\AutoRest directories, copied the AuthenticationChallengeParser.cs file, and the nuget.config I could better simulate what Samples Browser would ZIP up, which is how I found I needed to set another property in that last commit (it's set automatically when built inside our repo).

@heaths heaths merged commit 3d35400 into Azure:master Jun 5, 2021
@heaths heaths deleted the issue21543 branch June 5, 2021 00:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[QUERY] KeyVault: Sharelink sample can not be built in vs code
3 participants