SonarSource / sonar-dotnet

Code analyzer for C# and VB.NET projects
https://redirect.sonarsource.com/plugins/csharp.html
GNU Lesser General Public License v3.0
773 stars 226 forks source link

SQ Plugin: Remove RoslynProfileExporter #9564

Open mary-georgiou-sonarsource opened 1 month ago

mary-georgiou-sonarsource commented 1 month ago

Profile exporters have been deprecated and should be removed in the near future. *

Once the feature is removed from SQ, we should remove the RoslynProfileExporter class and its tests from the plugin. Note that S4MSB does not use this class since this commit..

I'll add a link to the jira ticket related to the profile exporter removal once it's created by the SQ platform team.

update after Pavel's comment below checking the deprecation policy on xtranet and discussing with Pavel: Exporters have been deprecated since 9.1 and, according to the SQ deprecation policy , can be removed in any version after 10.1.

This means we can safely remove them as of now.

pavel-mikula-sonarsource commented 1 month ago

We don't need to wait for the removal of the base functionality from products. We can remove this anytime after SQL 10.x LTA release. So this could be removed in 11.0 on our side

andrei-epure-sonarsource commented 1 month ago

Regarding the ticket (new) description

update after Pavel's comment below: Exporters have been deprecated since 9.1 and, according to the SQ deprecation policy , can be removed in any version after 10.1. This means we can safely remove them as of now.

The policy says:

Ideally, those changes should be done in 9.0 so that they can keep upgrading in 9.X without more breaking changes and for consistency with versioning best practices

Wouter said:

Our deprecation policy changed after the 9.9 LTS, so it could very well be removed sooner (11.0 at the earliest).

And Pavel said:

We can remove this anytime after SQL 10.x LTA release. So this could be removed in 11.0 on our side

But these changes will go in SonarQube 10.7, not in SonarQube 11.0. So it will be a breaking change for users moving from 10.6 to 10.7.

So why is this in progress and has a PR to close it?

mary-georgiou-sonarsource commented 1 month ago

@andrei-epure-sonarsource In the deprecation policy of SQ it's mentioned

Something deprecated in 8.X can only be removed in 9.X, to give enough time for users to adapt and so that users can choose when to upgrade.

Example: deprecate in 8.8 and remove in 9.0 - Not ideal but possible and would allow us to bring new features in the next major version. It also depends on how impactful those changes are and how long it takes for users to adapt to them.

This feature has been deprecated since 9.1 so my understanding is that we can remove it since 10.X. Additionally, this feature works with dependency injection, it appears because we publish it from the plugin, so it's on our side to remove it (SQ does not break, analysis does not break). So, being past the deprecation policy and not needing something from SQ IMO, we can safely remove this.

Let me know if I've understood something totally wrong.

Now, regarding this ticket in practice: the RoslynProfileExporter won't be removed in this release as the class is not only used to help with profile export in SQ but also to write files during the begin step of the scanner in the .sonarqube folder. My plan for now is to fully understand where and how this is happening - document it here so we can delete the needed code and tests and refactor accordingly (I'll update the open PR accordingly to make it clear, I forgot to do so on Wednesday). However, I'd like to clarify my understanding of the deprecation for future removal of features.

andrei-epure-sonarsource commented 1 month ago

My plan for now is to fully understand where and how this is happening - document it here so we can delete the needed code and tests and refactor accordingly (I'll update the open PR accordingly to make it clear, I forgot to do so on Wednesday).

Great.

Additionally, this feature works with dependency injection, it appears because we publish it from the plugin, so it's on our side to remove it (SQ does not break, analysis does not break).

If it's not used in any functionality exposed by SQ as an API (e.g. users downloading the profile for whatever reason) - then there's no problem in removing. You could send an email to our support colleagues - if they have no idea about this functionality, we can safely remove it.

However, I'd like to clarify my understanding of the deprecation for future removal of features.

The page says "Ideally, those changes should be done in 9.0 so that they can keep upgrading in 9.X without more breaking changes and for consistency with versioning best practices"

Versioning best practice means any breaking change requires a major version update. If we introduce breaking changes from 10.6 to 10.7, even if it's for removing deprecated APIs, it's can be a friction points for users updating.

All in all, here I think you are correct saying that from 9.1 users had plenty of time to see the deprecated API and react.