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

Change default BuildSnippets to check all snippets are used #33877

Closed
5 tasks
heaths opened this issue Feb 3, 2023 · 3 comments · Fixed by #33878
Closed
5 tasks

Change default BuildSnippets to check all snippets are used #33877

heaths opened this issue Feb 3, 2023 · 3 comments · Fixed by #33878
Assignees
Labels
Client This issue points to a problem in the data-plane of the library. EngSys This issue is impacting the engineering system.

Comments

@heaths
Copy link
Member

heaths commented Feb 3, 2023

The CI pipeline can check that all snippets are used to make sure people don't accidentally forget to use any they defined (and also makes sure any referenced are defined), but the default is turned off currently. Many SDKs have already opted in to do this check during the Analyze job, but I think we should enable that by default.

Running eng/scripts/Update-Snippets.ps1 * -StrictMode, I find the following SDKs still have some unreferenced snippets:

  • communication
  • dns
  • formrecognizer
  • iot
  • servicebus

We should make sure that a new instance of a DPG template does not run afoul of this error as well, but given the command above didn't find any issues it should be fine. We should create an instance though and double check.

Client library authors should run Update-Snippets.ps1 {ServiceDirectory} to check compliance, and to make sure any README or sample markdowns are properly updated (which the Analyze step will also do or fail).

Also see related snippet clean-up issue #20431

@heaths heaths added Client This issue points to a problem in the data-plane of the library. EngSys This issue is impacting the engineering system. labels Feb 3, 2023
@heaths heaths self-assigned this Feb 3, 2023
@heaths
Copy link
Member Author

heaths commented Feb 3, 2023

/cc @JoshLove-msft @m-nash

@heaths
Copy link
Member Author

heaths commented Feb 3, 2023

If we do change the default, we can modify each of these service directories' ci.yml files to pass BuildSnippet: false. See

BuildSnippets: true
for an example. We should also go through and get rid of such lines so just so it's more obvious we check snippets by default and isn't something you need to opt into.

Also /cc @pallavit @jsquire

heaths added a commit to heaths/azure-sdk-for-net that referenced this issue Feb 3, 2023
@m-nash
Copy link
Member

m-nash commented Feb 3, 2023

I would be a fan of turning this on by default. Today I have to manually check these things (didn't know about the opt in flag) and its easy to miss them.

heaths added a commit that referenced this issue Feb 6, 2023
* Build snippets during Analyze job by default

Resolves #33877

* Resolve PR feedback

* Don't build snippets for Core

Core has a few issues actually compiling with SNIPPET symbol because it
tries to elide some code for brevity. Fixing that changes the extracted
snippets and may make the samples more confusing so leaving as-is.
@github-actions github-actions bot locked and limited conversation to collaborators May 7, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Client This issue points to a problem in the data-plane of the library. EngSys This issue is impacting the engineering system.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants