-
Notifications
You must be signed in to change notification settings - Fork 183
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 mgmt toc structure #3428
Conversation
The following pipelines have been queued for testing: |
@@ -266,12 +268,12 @@ if ($otherPackages) { | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One comment above is referring to GetClientPackageNode you should update it to match the new name.
$serviceReadmeBaseName = $service.ToLower().Replace(' ', '-').Replace('/', '-') | ||
$mgmtItems = @() | ||
foreach ($pkg in $mgmtPackages) { | ||
# $children = &$GetDocsMsTocChildrenForManagementPackagesFn ` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this function going away or do you plan to call it again?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function is used in the Java implementation to fetch namespaces from Preview and GA packages (covering the case that a package is introducing new namespaces we want to show all namespaces in the relevant ToC) -- https://github.com/Azure/azure-sdk-for-java/blob/main/eng/scripts/docs/Docs-ToC.ps1#L54
... I think we should keep it so we get an accurate set of namespaces for a given package.
On closer inspection I think we can get rid of it because we're already doing something similar here: https://github.com/Azure/azure-sdk-for-java/blob/main/eng/scripts/docs/Docs-ToC.ps1#L12
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I planed to remove. The PR is in early stage. Will focus this PR once service level readme is ready
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
High level seems reasonable but I will defer to @danieljurek for sign-off.
# There could be multiple packages, ensure this is treated as an array | ||
# even if it is a single package | ||
children = @($children) | ||
href = "~/docs-ref-services/{moniker}/$serviceReadmeBaseName.md" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this insert the service overview into the ToC underneath management?
- ...
- Key vault
- Overview (service level overview, expected)
- Keys
- Overview (package level overview)
- @azure/keyvault-keys (package ref docs)
- ...
- Management
- Overview <======== (Is this re-inserting the service level overview?)
- Key Vault Management (whatever the package DisplayName is)
- Overview (package level overview)
- @azure/arm-keyvault (package ref docs)
- ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch!!
The following pipelines have been queued for testing: |
The following pipelines have been queued for testing: |
The following pipelines have been queued for testing: |
e7d4031
to
6e920d2
Compare
The following pipelines have been queued for testing: |
The following pipelines have been queued for testing: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes to Update-DocsMsToc.ps1
look good 👍
One small change to Service-Level-Readme-Automation.ps1
and this is good to go.
The following pipelines have been queued for testing: |
The following pipelines have been queued for testing: |
The following pipelines have been queued for testing: |
The following pipelines have been queued for testing: |
Hello @azure-sdk! Because this pull request has the p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (
|
Our current mgmt toc structure does not have readme in place.
arm packages have their dedicated readme copied to docs repo. It is not linked anywhere in toc.
The PR is to align mgmt with client packages.
Proposed structure:
Here is the testing review site:
https://review.docs.microsoft.com/en-us/javascript/api/overview/azure/?view=azure-node-latest&branch=daily%2F2022-05-02-ci-succeeded
Testing in other langs:
Diff in Java:
Azure/azure-docs-sdk-java@a162d01
Review site: https://review.docs.microsoft.com/en-us/java/api/overview/azure/?view=azure-java-stable&branch=main
Diff in Python:
MicrosoftDocs/azure-docs-sdk-python@46b9cff#diff-66355581b7f76c540b239a41f1f63242c7a14380a03f10ed943a758bc03ac6c7
Review site:
https://review.docs.microsoft.com/en-us/python/api/overview/azure/mgmt-advisor-readme?view=azure-python&branch=daily%2F2022-05-02-ci-succeeded