-
Notifications
You must be signed in to change notification settings - Fork 25.1k
[BULK CHANGE] Content SFI - Stay Green: Add ms.custom values for ROPC, GA, and Images #35850
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
[BULK CHANGE] Content SFI - Stay Green: Add ms.custom values for ROPC, GA, and Images #35850
Conversation
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.
Hello @Dickson-Mwendia ... Please don't change the order of the metadata. Make the changes only to the value when ms.custom is present. If the entry isn't present, it goes after ms.author (they're alphabetical after the title). Thx!
|
No worries, I used a script to pull these values from the security dashboard and insert them into the metadata blocks; but I can address this. Thanks @guardrex |
|
Also, why break out on separate lines when a comma-separated list will suffice and reduces scrolling? |
This is more of a lack of consistency in how we handle metadata across Microsoft docs repos. We're running the script across 200+ repos, and somehow there's no single way on how it's done. We had to standardize how we insert the values to avoid busted entries in some cases. |
I can explain that ...... but I'm not going to. 😆 I defer to @tdykstra and @wadepickett on if we should run our own script after this merges to collapse them back to one line. |
|
Closing and re-opening to force a new content build since some Identity related API Ref that was missing from another team was just fixed, which should take care of the cross-reference warnings... |
|
Closing and re-opeining to force a new content build since some Identity related API Ref that was missing from another team was just fixed. |
|
@Dickson-Mwendia and @guardrex, closing and reopening the build took care of all the cross-link API Reference warnings, but there is a merge conflict error: [Error] Cannot merge commit a60e7a7 in branch dmwendia-sfi-mscustom of repository https://github.com/Dickson-Mwendia/AspNetCore.Docs into branch main (commit f275bd5). Please follow this documentation: https://help.github.com/articles/resolving-a-merge-conflict-using-the-command-line/ to use git.exe to resolve you content conflicts locally and then push to remote. @guardrex, it looks like a merge conflict on a blazor doc. I defer to you, since you will recognize right away what stays in. |
Unless we find it breaks something later, I think we should leave the ms.custom in the format used throughout the org so that as they need to be updated later it isn't a pain for the SFI team. |
|
I'll see about resolving the conflicts. Give me few minutes ... I'm just wrapping up something.
Of course, whatever MS decides is fine with me, but I observe that ...
|
|
Merge commits are resolved. WRT the metadata ordering, we had a convention to follow that worked well ... it's kind'a gone to heck in a handbasket over the years in the main doc set, but I still consistently use it across the Blazor node to keep Blazor article management cost effective 💰 for Microsoft. I'll discuss the situation with DR offline. |
|
"They can easily script for single-line changes. It's simple." |
wadepickett
left a comment
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.
Looks great, thanks @Dickson-Mwendia! Thanks @guardrex for the quick merge conflict fix!
|
I'll remove myself from review to let MS do this 💸. |
|
@guardrex, if you still think this PR needs a change, we can work that out. I didn't mean for my own approval to rule that out. |
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 mistakenly thought that addressing the alpha order of the metadata had already happened.
@Dickson-Mwendia were you going to fix that? It is a guideline we have as an org, but we also want to be wary that you are dealing with 200+ repos and trying to get this done. There is also a July deadline, and it is 7/30, so I am keeping that in awareness as well since this is a very last minute concern.
cc @guardrex
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.
Hum 🤔 ... GH doesn't permit dropping the change request by removing the review request. I'll try to "Comment" to remove it.
Ah! ... I forgot the "dismiss review" process.
|
The deadline for this is tomorrow. If we have to we can fix after this PR, understanding that @Dickson-Mwendia is trying to herd 200+ repos to completion by that time. @Dickson-Mwendia if you can fix for this PR today, great. If it is problematic given the scale of what you are trying to pull over the finish line by tomorrow, we can certainly consider that. |
|
@wadepickett @guardrex thank you so much for the review and feedback here. I'll come back and fix the metadata order tomorrow (7/31). Let's hold off merging for today. I'll ping you both for sign-off once done. |
|
@wadepickett @guardrex I've addressed the issues discussed and this is now ready for review. |
guardrex
left a comment
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.
Thanks, @Dickson-Mwendia, for doing that. There's just two spots to flip the author from Rick back to Tom. Other than that, LGTM! 🎉
Co-authored-by: Luke Latham <[email protected]>
Perfect, this is now addressed, @guardrex |
wadepickett
left a comment
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.
Looks great. Thanks @Dickson-Mwendia and @guardrex!
|
We still have xref issues for what I think is missing API from the Preview 5 & 6 API ref build that was on hold because it had a mess of errors. Should we squash and merge this anyway? xref-not-found aspnetcore/mvc/models/file-uploads.md Cross reference not found: 'Microsoft.Azure.Storage.File.CloudFile.UploadFromFileAsync*'. |
|
Yes, those are the ✨ NEW ✨ MIA API entries. They exist, so it's another XREF/build problem. |
Internal previews
Toggle expand/collapse
web.configfileNote
This table shows preview links for the 30 files with the most changes. For preview links for other files in this PR, select OpenPublishing.Build Details within checks.