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

Markdownlint GitHub #122

Merged
merged 10 commits into from
Nov 13, 2024
Merged

Markdownlint GitHub #122

merged 10 commits into from
Nov 13, 2024

Conversation

Gijsreyn
Copy link
Contributor

@Gijsreyn Gijsreyn commented Nov 7, 2024


Microsoft Reviewers: Open in CodeFlow

@Gijsreyn
Copy link
Contributor Author

Gijsreyn commented Nov 7, 2024

@denelon

This comment has been minimized.

Copy link
Contributor

@Trenly Trenly left a comment

Choose a reason for hiding this comment

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

Can you help me understand how this action works? Is it like the spell checking where it fails if the linting doesn't match, or is it something that actually goes in and fixes the linting?

.github/workflows/markdownLint.yaml Show resolved Hide resolved
- '**'
pull_request_target:
branches:
- '**'
Copy link
Contributor

@Trenly Trenly Nov 7, 2024

Choose a reason for hiding this comment

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

Should we be running on every pull request, or should it be only those that target into main ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess only for the PRs, the same as spell checker?

@Gijsreyn
Copy link
Contributor Author

Gijsreyn commented Nov 7, 2024

Can you help me understand how this action works? Is it like the spell checking where it fails if the linting doesn't match, or is it something that actually goes in and fixes the linting?

The same as the spell checker. I didn't explicitly set the continueOnError, because $false is the default behaviour.

@denelon
Copy link
Contributor

denelon commented Nov 11, 2024

I saw some of the "security" related items in one of the files. We want folks to submit security related concerns through the official Microsoft link mentioned in SECURITY.md.

@Gijsreyn
Copy link
Contributor Author

I saw some of the "security" related items in one of the files. We want folks to submit security related concerns through the official Microsoft link mentioned in SECURITY.md.

I didn't fully get what you mean Demitrius. You mind elaborating a bit more what you sawa and what might needs to be changed?

@denelon
Copy link
Contributor

denelon commented Nov 12, 2024

I didn't fully get what you mean Demitrius. You mind elaborating a bit more what you sawa and what might needs to be changed?

I reviewed it, I think it's fine.

@Gijsreyn
Copy link
Contributor Author

@denelon Thanks! @ryfu-msft If you're also happy, you mind pulling it in?

@ryfu-msft
Copy link
Contributor

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@Gijsreyn
Copy link
Contributor Author

Ryan, I don't know why that prerelease is failing, whereas other runs it is successful. No file has been touched on tests. Can we create a separate issue for it for investigation and pull this in?

@ryfu-msft
Copy link
Contributor

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@ryfu-msft
Copy link
Contributor

Ryan, I don't know why that prerelease is failing, whereas other runs it is successful. No file has been touched on tests. Can we create a separate issue for it for investigation and pull this in?

Lets try one more time and see if its a one off.

@Gijsreyn
Copy link
Contributor Author

At least from my side:
image

@Gijsreyn
Copy link
Contributor Author

Gijsreyn commented Nov 13, 2024

I see what is going wrong, but I don't know if it is a bug in the DSC resource, or something on the dotnet.exe NuGet side. Version 9.0.0 has just been released, and it's picking that one. Can we comment out the test and I'll take a look at it in another issue? We still have one test that test for prerelease, which is PowerShell.

EDIT: When I run dotnet tool install dotnet-ef --prerelease --global, I would assume it is going to install: 9.0.0-rc.2.24474.1, but it doesn't.

EDIT 2: Looking at the description, not really:

image

@ryfu-msft Would the PreRelease property then make sense, or just instruct users if they want to install a rc for example, just add the full version?

@ryfu-msft
Copy link
Contributor

I see what is going wrong, but I don't know if it is a bug in the DSC resource, or something on the dotnet.exe NuGet side. Version 9.0.0 has just been released, and it's picking that one. Can we comment out the test and I'll take a look at it in another issue? We still have one test that test for prerelease, which is PowerShell.

EDIT: When I run dotnet tool install dotnet-ef --prerelease --global, I would assume it is going to install: 9.0.0-rc.2.24474.1, but it doesn't.

EDIT 2: Looking at the description, not really:

image

@ryfu-msft Would the PreRelease property then make sense, or just instruct users if they want to install a rc for example, just add the full version?

Seems like --prerelease only works if there is a prerelease version that exists later the latest stable release. I would instruct users to resort to using the full version instead of --prerelease since you never know when the next preview release will come out.

Regarding the tests, it would be better to explicitly point to an older preview version so that the tests don't break in the future when a new release comes out.

In an ideal world, we should not be using real packages from the nuget gallery but instead stand up our own test package source.

@ryfu-msft ryfu-msft merged commit 6c62e64 into microsoft:main Nov 13, 2024
5 of 7 checks passed
@Gijsreyn Gijsreyn deleted the markdownlint-github branch November 13, 2024 18:50
@Gijsreyn
Copy link
Contributor Author

I fully agree with you here Ryan. I'll hit up a small commit in just a second to fix the test and the one for VSCode. If you want, I can remove the --prerelease later. Otherwise, I put it in the docs now. Thanks for checking.

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.

4 participants