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

Fix markdown-link-check to run properly, fix JSON parse warning in CredScan #8325

Merged
merged 8 commits into from
Jul 18, 2022

Conversation

MiYanni
Copy link
Member

@MiYanni MiYanni commented Jul 15, 2022

This PR fixes our markdown-link-check GitHub action. It has (apparently) been broken since I added it. This was caused by an improperly encoded ignorePattern. These patterns are RegEx. However, the patterns also need to respect JSON string rules. The changes include:

  • Fixing broken ignorePattern
  • Adding additional ignorePatterns as necessary
  • Fix broken URLs
  • Remove language encoding from URLs
    • Like /en/ or /en-us/
    • These appropriately cause the link check to fail as they should not be present in links
  • Ran into an issue with one of our links, worked around it, and appropriately filed a bug against markdown-link-check

I've added code comments on some of the changes. I can't add those comments into the file itself since JSON doesn't natively support commenting.

This issue was initially to fix a warning I saw in our CredScan runs for our compliance pipeline. These changes should also fix that warning.
image

The parsing issue mentioned in the warning is also the same reason the checker wasn't working before this. Ironically, the checker itself wasn't throwing a warning about it.

Microsoft Reviewers: Open in CodeFlow

@@ -23,7 +23,7 @@ _Descriptions of these steps can be found in the team OneNote._
- [ ] Update the [signed build definition](https://devdiv.visualstudio.com/DevDiv/_build?definitionId=9675) to build the new branch
- [ ] Update Roslyn Tools [config.xml](https://github.com/dotnet/roslyn-tools/blob/main/src/GitHubCreateMergePRs/config.xml) file to flow branch changes to the latest dev branch
- [ ] dotnet/roslyn-tools PR: https://github.com/dotnet/roslyn-tools/pull/❓
- [ ] Update [Versions.props](..\..\build\import\Versions.props) (via `<ProjectSystemVersion>`) and [version.json](..\..\version.json) (via `"version"` and `"assemblyVersion"`) to match the version of VS, if needed
- [ ] Update [Versions.props](..\\..\build\import\Versions.props) (via `<ProjectSystemVersion>`) and [version.json](..\\..\version.json) (via `"version"` and `"assemblyVersion"`) to match the version of VS, if needed
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would / work instead?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would likely work, yes. However, I've made every local link in the repo use \. I'd have to go through and update all of them. I'll consider doing that at some point.

@@ -1,13 +1,30 @@
{
"ignorePatterns": [
{
"pattern": "^https://.*\.visualstudio\.com"
"pattern": "^https:\/\/.*\\.visualstudio\\.com"
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Backslash prior to the RegEx escaping backslash is required for JSON string encoding.

"headers": {
"Accept-Encoding": "zstd, br, gzip, deflate"
}
}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Workaround for docs.github.com links. See: tcort/markdown-link-check#201 (comment)

@@ -3,7 +3,7 @@ on: [push, pull_request]

jobs:
markdown-link-check:
name: verify
name: markdown-link-check
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make the name match the action when viewing it in the Action menu.
image

@MiYanni MiYanni changed the title Fix CredScan warning about markdown-link-check-config.json Fix markdown-link-check to run properly, fix JSON parse warning in CredScan Jul 16, 2022
@MiYanni MiYanni marked this pull request as ready for review July 16, 2022 00:42
@MiYanni MiYanni requested a review from a team as a code owner July 16, 2022 00:42
- [official.yml](build\official.yml): This file is our official build pipeline that produces the signed packages (VSIX) that used for insertion into Visual Studio.
- [unit-tests.yml](build\unit-tests.yml): This file is our build pipeline used when validating pull requests into the repo from GitHub.
- [official.yml](ci\official.yml): This file is our official build pipeline that produces the signed packages (VSIX) that used for insertion into Visual Studio.
- [unit-tests.yml](ci\unit-tests.yml): This file is our build pipeline used when validating pull requests into the repo from GitHub.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I take it this is fixing a broken link that is now being reported as broken?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct. This PR both fixed reporting broken links and fixed the links. If you look at the failed commits in this PR, you can see the runs of the GitHub action, reporting links like this one being broken. Click on the little red ❌ from the commits above to see those runs:
image

@MiYanni MiYanni merged commit 227ad28 into dotnet:main Jul 18, 2022
@ghost ghost added this to the 17.4 milestone Jul 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants