Add ToC Generation script and wire up to docindex.yml by danieljurek · Pull Request #19915 · Azure/azure-sdk-for-js

danieljurek · 2022-01-19T20:01:11Z

Docindex running and producing the ToC -- https://dev.azure.com/azure-sdk/internal/_build/results?buildId=1315921&view=logs&j=dc056dfc-c0cf-5958-c8c4-8da4f91a3739&t=98c76ae6-047b-5869-f98c-bcf6099a854a

Review site with new ToC layout -- https://review.docs.microsoft.com/en-us/javascript/api/overview/azure/?branch=daily%2F2022-01-20-toc-test&view=azure-node-latest

Please have a look at

eng/common/scripts/Update-DocsMsToc.ps1
eng/scripts/Language-Settings.ps1
eng/scripts/docs/Docs-ToC.ps1

weshaggard · 2022-01-21T01:08:30Z

eng/common/scripts/Update-DocsMsToc.ps1

+
+# Map metadata to package. Validate metadata entries.
+$packagesForToc = @{}
+foreach ($packageName in $onboardedPackages.Keys) {


Not sure how or if it is worth it sharing this but if we do need the same thing for other languages we should add a helper. I also have a similar helper in the azure-sdk repo https://github.com/Azure/azure-sdk/blob/main/eng/scripts/PackageList-Helpers.ps1#L178.

This looks like it can work. It might make sense to put it in eng/common/scripts though its usage is only scoped to Update-DocsMsToc.ps for now.

Yeah something under eng-common that builds a look-up table once we have multiple users.

eng/common/scripts/Update-DocsMsToc.ps1

eng/scripts/docs/Docs-ToC.ps1

eng/common/scripts/Update-DocsMsToc.ps1

benbp · 2022-01-24T19:17:31Z

Some minor nits otherwise I'll defer to @weshaggard here.

eng/common/pipelines/templates/steps/set-daily-docs-branch-name.yml

eng/common/scripts/Update-DocsMsToc.ps1

weshaggard · 2022-01-24T19:34:33Z

eng/scripts/docs/Docs-ToC.ps1

+  $packageLevelReadmeName = $packageMetadata.Package.Replace('@azure/', '').Replace('@azure-tools/', '').Replace('azure-', '');
+
+  # Fallback to get package-level readme name if metadata file info does not exist
+  if ($packageMetadata.Package.StartsWith('@azure-rest/')) {


It is unfortunate that we another place to special case azure-rest. @praveenkuttappan something to look at with your other clean-up of that processing.

weshaggard

Added a few minor clean-up points but once your testing it done it seems good to me.

weshaggard · 2022-02-03T17:52:37Z

eng/common/scripts/Update-DocsMsToc.ps1

 if ($otherPackages) {
  foreach ($otherPackage in $otherPackages) {
-    $otherPackageItems += GetClientPackageNode $otherPackage
+    $segments = $otherPackage.DisplayName.Split(' - ')


Should we split only on "-" and then trim any whitespace? Just to make this a little more tolerable to simple typos.

weshaggard · 2022-02-03T17:53:31Z

eng/common/scripts/Update-DocsMsToc.ps1

+
+    if ($segments.Count -gt 1) {
+      $currentNode = $otherPackageItems
+      foreach ($segment in $segments[0..($segments.Count - 2)]) {


Why count - 2?

I know you added the comment to help address this question, but I think I'm still missing something. Maybe give an example of a display name that would illustrates what you are trying to do here.

If I understand correct we might end up with something like "Core - Http" and is the goal that we essentially drop the Http part and because we want that to be a leaf node and not nested node?

Correct. This loop does not build the leaf nodes. It only builds the branches extending from the "Other" node up to where the leaf should be added.

In the case of DisplayName "Core - Http"

- name: Other landingPageType: Service items: # $otherPackages # Built by the loop - name: Core landingPageType: Service items: # Added outside of the loop - name: Http href: ~/some/md/file.md children: - @azure/core-http

In the case of DisplayName "Core - Client - Http"

- name: Other landingPageType: Service items: # $otherPackages # Built by the loop - name: Core landingPageType: Service items: # Also built by the loop - name: Client landingPageType: Service items: # Added outside of the loop - name: Http href: ~/some/md/file.md children: - @azure/core-http

eng/common/scripts/Update-DocsMsToc.ps1

weshaggard · 2022-02-07T17:57:48Z

eng/common/scripts/Update-DocsMsToc.ps1

+        }
+      }
+
+      if ($null -ne $currentNode) {


Is it possible to have currentNode be null? If it is wouldn't that be an error which we should probably fail on?

We set $currentNode to $null when the metadata contains a hypothetical scenario like this:

Package DisplayName ServiceName

@azure/core-client Core - Client Other

@azuure/core-xml Core - Client - XML Other

In this case, we would build a node like this for the first:

- name: Core landingPageType: Service items: - name: Client href: ~/some/markdown/file.md children: - @azure/core-client

When we attempt to iterate in on the second node we reach Client and see that it is a leaf node (i.e. children instead of items). In this case we break out of that loop but need a way to say "Don't set an items property on this entry in the ToC, it won't work" ... Setting the node to $null is the way to do this.

If we quit the entire process here then we'd end up stopping the ToC update process and blocking on a package because of bad metadata. If we skip a package here that package will still end up in Uncategorized Packages and we'll be able to build the rest of the ToC.

If we skip a package here that package will still end up in Uncategorized Packages and we'll be able to build the rest of the ToC.

I guess I'm missing where this happens.

Originally proposed here -- Azure/azure-sdk-for-js#19915

check-enforcer · 2022-02-23T04:23:00Z

This pull request is protected by Check Enforcer.

What is Check Enforcer?

Check Enforcer helps ensure all pull requests are covered by at least one check-run (typically an Azure Pipeline). When all check-runs associated with this pull request pass then Check Enforcer itself will pass.

Why am I getting this message?

You are getting this message because Check Enforcer did not detect any check-runs being associated with this pull request within five minutes. This may indicate that your pull request is not covered by any pipelines and so Check Enforcer is correctly blocking the pull request being merged.

What should I do now?

If the check-enforcer check-run is not passing and all other check-runs associated with this PR are passing (excluding license-cla) then you could try telling Check Enforcer to evaluate your pull request again. You can do this by adding a comment to this pull request as follows:
/check-enforcer evaluate
Typically evaulation only takes a few seconds. If you know that your pull request is not covered by a pipeline and this is expected you can override Check Enforcer using the following command:
/check-enforcer override
Note that using the override command triggers alerts so that follow-up investigations can occur (PRs still need to be approved as normal).

What if I am onboarding a new service?

Often, new services do not have validation pipelines associated with them, in order to bootstrap pipelines for a new service, you can issue the following command as a pull request comment:
/azp run prepare-pipelines
This will run a pipeline that analyzes the source tree and creates the pipelines necessary to build and validate your pull request. Once the pipeline has been created you can trigger the pipeline using the following comment:
/azp run js - [service] - ci

eng/scripts/docs/Docs-ToC.ps1

benbp · 2022-02-23T20:42:07Z

eng/scripts/docs/Docs-ToC.ps1

Is it expected that there would be multiple matches for this in $items?

There will not be multiple matches for a service in this way given how the ToC is generated from a hashed set of service names.

benbp · 2022-02-23T20:43:08Z

eng/scripts/docs/Docs-ToC.ps1

Is this intermediate variable necessary since you're just updating by reference? I think it's a little confusing since there are so many nested items properties, vs. just using $toc[$i].items += ... below.

$items[$i].name is easier to read than $toc[0].items[$i].name when repeated. However, a better name for this intermediate variable is probably $services which applies well enough in this domain.

I like $services better yeah.

Add Docs-ToC.ps1 Remove eng/scripts/docs from .gitignore but keep filtering other docs/ folders Also checkout packages-legacy.json in sparse checkout Also check out docs-ref-mapping/ Use -toc-test branch for demo Review feedback Review feedback Remove unnecessary comment Undo working changes Add extension point to update the ToC before output Undo changes to demo set-daily-docs-branch-name.yml Check for FileMetadata before accessing it Add ability to artbitarily nest packages in the 'Other' service. Add comments from review feedback Add plugin nodes

danieljurek marked this pull request as ready for review January 20, 2022 22:43

danieljurek requested review from benbp, mikeharder, praveenkuttappan and weshaggard as code owners January 20, 2022 22:43

weshaggard reviewed Jan 21, 2022

View reviewed changes

eng/common/scripts/Update-DocsMsToc.ps1 Outdated Show resolved Hide resolved

weshaggard reviewed Jan 21, 2022

View reviewed changes

eng/common/scripts/Update-DocsMsToc.ps1 Show resolved Hide resolved

weshaggard reviewed Jan 21, 2022

View reviewed changes

eng/common/scripts/Update-DocsMsToc.ps1 Outdated Show resolved Hide resolved

weshaggard reviewed Jan 21, 2022

View reviewed changes

eng/common/scripts/Update-DocsMsToc.ps1 Show resolved Hide resolved

weshaggard reviewed Jan 21, 2022

View reviewed changes

eng/scripts/docs/Docs-ToC.ps1 Outdated Show resolved Hide resolved

benbp reviewed Jan 24, 2022

View reviewed changes

eng/common/scripts/Update-DocsMsToc.ps1 Outdated Show resolved Hide resolved

benbp reviewed Jan 24, 2022

View reviewed changes

eng/common/scripts/Update-DocsMsToc.ps1 Outdated Show resolved Hide resolved

weshaggard reviewed Jan 24, 2022

View reviewed changes

eng/common/pipelines/templates/steps/set-daily-docs-branch-name.yml Outdated Show resolved Hide resolved

weshaggard reviewed Jan 24, 2022

View reviewed changes

eng/common/scripts/Update-DocsMsToc.ps1 Show resolved Hide resolved

weshaggard reviewed Jan 24, 2022

View reviewed changes

weshaggard approved these changes Jan 24, 2022

View reviewed changes

weshaggard reviewed Feb 3, 2022

View reviewed changes

eng/common/scripts/Update-DocsMsToc.ps1 Show resolved Hide resolved

weshaggard reviewed Feb 7, 2022

View reviewed changes

danieljurek mentioned this pull request Feb 15, 2022

Add logic to generate docs.ms ToC Azure/azure-sdk-tools#2759

Merged

ghost pushed a commit to Azure/azure-sdk-tools that referenced this pull request Feb 17, 2022

Add logic to generate docs.ms ToC (#2759)

0095008

Originally proposed here -- Azure/azure-sdk-for-js#19915

danieljurek force-pushed the djurek/generate-docms-toc branch from 5f8a6d4 to 9a1dcba Compare February 23, 2022 04:12

benbp mentioned this pull request Feb 23, 2022

Migrate JS metadata to generate desired ToC layout Azure/azure-sdk#3942

Merged

benbp reviewed Feb 23, 2022

View reviewed changes

eng/scripts/docs/Docs-ToC.ps1 Outdated Show resolved Hide resolved

benbp reviewed Feb 23, 2022

View reviewed changes

danieljurek force-pushed the djurek/generate-docms-toc branch from 9a1dcba to e754ebc Compare February 24, 2022 04:17

danieljurek added 7 commits February 24, 2022 11:32

Revert unnecessary changes to eng/common

3ab6bb5

Review feedback

231b077

Remove automatic ToC generation from main

86cf2f7

Set-StrictMode

9094ad7

Remove Release-DocsMsDocs.ps1

247bc0d

Use original syntax

f50862f

danieljurek force-pushed the djurek/generate-docms-toc branch from d38cff5 to f50862f Compare February 24, 2022 19:32

benbp self-requested a review February 24, 2022 20:24

benbp approved these changes Feb 24, 2022

View reviewed changes

danieljurek merged commit a291861 into main Mar 1, 2022

danieljurek deleted the djurek/generate-docms-toc branch March 1, 2022 15:46

Package	DisplayName	ServiceName
@azure/core-client	Core - Client	Other
@azuure/core-xml	Core - Client - XML	Other

Comments

Conversation

danieljurek commented Jan 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

benbp commented Jan 24, 2022

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

weshaggard left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danieljurek Feb 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danieljurek Feb 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

check-enforcer bot commented Feb 23, 2022

What is Check Enforcer?

Why am I getting this message?

What should I do now?

What if I am onboarding a new service?

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

danieljurek commented Jan 19, 2022 •

edited

Loading

danieljurek Feb 7, 2022 •

edited

Loading

danieljurek Feb 7, 2022 •

edited

Loading