-
-
Notifications
You must be signed in to change notification settings - Fork 242
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
Upgrade dotnet to v6 #1680
Upgrade dotnet to v6 #1680
Conversation
* Adding dotnet project files * Better .cs examples of bad code Co-authored-by: Alexandre Melotti <contato@lexmelotti.com>
…o features/dotnet-upgrade-proposed-changes # Conflicts: # megalinter/descriptors/csharp.megalinter-descriptor.yml # megalinter/descriptors/vbdotnet.megalinter-descriptor.yml
Everything should be okay now, the tests failing indicate that they're still expecting old dotnet 5.0 dotnet-format paremeters (with Isn't there anywhere else that has to upgrade from dotnet 5.0 to dotnet 6.0? |
@lextatic my bad, DevSkim and tsqllint use dotnet too... and I've not factorized such commands yet... I forgot to update their descriptors ! Install commands are the following, are they still ok for dotnet v6 ?
|
…o features/dotnet-upgrade-proposed-changes
I wouldn't know, will have to look into it. I think we're still missing something. Edit: I think it's alright -- https://docs.microsoft.com/en-us/dotnet/core/tools/dotnet-install-script |
@nvuillam Forgot to tag you in the previous comment, but I went on and decided to remove the And there is still an issue with Terraform/Terrascan??? Can you check into that as well? I had to sort the |
…pgrade-proposed-changes
@lextatic sorru for the delay... go for dotnet v7 if it has retrocompability with v6 :) ( I wouldn't like dotnet6 users to not be able to use ML ) |
…pgrade-proposed-changes # Conflicts: # CHANGELOG.md # Dockerfile # flavors/dotnet/Dockerfile # megalinter/descriptors/csharp.megalinter-descriptor.yml
Codecov Report
@@ Coverage Diff @@
## main #1680 +/- ##
==========================================
+ Coverage 82.97% 82.99% +0.02%
==========================================
Files 170 170
Lines 4469 4470 +1
==========================================
+ Hits 3708 3710 +2
+ Misses 761 760 -1
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
@lextatic note that the csharpier installation is set to 0.16.0 because in the main branch we are on .NET 5 but when you upgrade to .NET 7 you will probably have to remove "--version 0.16.0" to install the latest version. |
Thanks, I'm removing the version arguments from the CLI but I think more stuff had failed because of the upgrade. Feel free to contribute with this PR if you know how to handle it. If we can't do it we can revert it back to .NET 6 as it was working fine and passing all tests there. Keeping it as .NET 6 isn't that bad since .NET 6 is the LTS and .NET 7 is just a STS, meaning both will be supported until the release of .NET 8. |
Agreed that LTS is much netter than STS, i thought dotnet v7 was also also lts bit if it's not the case it's probably better to remain on dotnet v6 |
@nvuillam I don't know if it is relevant to the use we make of it but just to keep it in mind: https://devblogs.microsoft.com/dotnet/performance_improvements_in_net_7/ |
It depends. If all the tools we need can run 7, we can use 7, dotnet always increases performance (it is always checked when integrating new changes, if it slows downs it isn't really accepted). I don't think that the dotnet tools need to run the user's repo dotnet code, so we use the version we want. |
@bdovaz same thoughts |
If v7 is STS, i don't know about the user adoption And as i do not use dotnet, i have no idea 😅 All I know for sure is that we need to stop being with dotnet v5 ^^ |
The only difference is the support time: https://dotnet.microsoft.com/en-us/platform/support/policy In terms of stability and quality they assure that they have the same standards, it's not that .NET 7 is "beta" or something similar. |
And we know a new one will exist after that. |
So like @echoix said, if all the apps we use are compliant, we could go with v7 and switch to v8 that will be released long before the end of v7 support, indeed :) |
This reverts commit 7630633.
I've made some tests and it seems like Devskim is blocking us again, also some other tests are failing that would need more time to investigate why. So how about that? Since it's already working fine with v6 we proceed with it for this PR and further investigate if we can upgrade to v7 on a future PR. At least this way we're out of v5 and on to a LTS version of .NET. |
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.
Great job , many many thanks for this huge contribution :)
@lextatic i think DevSkim is simply not compliant with v7... it's compliant with v6 for a few time, and after my request ^^ |
@lextatic @nvuillam the alpha versions are compatible with .NET 7, you can test them in a PR in draft if you want and wait until that version is stable: https://www.nuget.org/packages/Microsoft.CST.DevSkim.CLI/0.8.8-alpha#supportedframeworks-body-tab |
I think we'll wait all official releases are compliant with v7, else we'll wait for v8 :) |
Added the project files for the
sample_project_fixes
tests.But I think it won't be enough. It seems the command is being executed from the root of the repository, so it has to specify the workspace path where the project/sln file is located as the first CLI parameter when executing. Is something like this possible in the descriptor?
Maybe like this? (following what I saw in switft and repository descriptors)