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

SLCORE-999 Store custom impacts when synchronizing rules #1141

Conversation

eray-felek-sonarsource
Copy link
Contributor

@eray-felek-sonarsource eray-felek-sonarsource commented Oct 28, 2024

backend/server-api/src/main/proto/sonarqube/ws-rules.proto Outdated Show resolved Hide resolved
return new ServerActiveRule(ruleOrTemplateDefinition.get().getKey(), possiblyDeprecatedActiveRuleFromStorage.getSeverity(),
possiblyDeprecatedActiveRuleFromStorage.getParams(),
null, Collections.emptyList());
null, possiblyDeprecatedActiveRuleFromStorage.getOverriddenImpacts());
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This method is very similar to the one in AnalysisService, maybe there is a way to keep only one

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How about making it static on RuleService and using it on AnalysisService?

@eray-felek-sonarsource eray-felek-sonarsource force-pushed the feature/ef/SLCORE-999-store-custom-impacts branch from 1294390 to fc21718 Compare October 30, 2024 09:55
@eray-felek-sonarsource eray-felek-sonarsource force-pushed the feature/ef/SLCORE-999-store-custom-impacts branch 2 times, most recently from 282bd48 to 9de7393 Compare October 30, 2024 12:23
Copy link
Contributor

@damien-urruty-sonarsource damien-urruty-sonarsource left a comment

Choose a reason for hiding this comment

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

Some improvement ideas, but overall LGTM

@@ -147,11 +149,25 @@ public static RuleDetails merging(ServerActiveRule activeRuleFromStorage, Server
serverRule.getSeverity(),
templateRuleDefFromPlugin.getType(),
cleanCodeAttribute,
defaultImpacts,
mergeImpacts(defaultImpacts, activeRuleFromStorage.getOverriddenImpacts()),
Copy link
Contributor

Choose a reason for hiding this comment

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

I missed that ServerActiveRule.getOverridenImpacts() returns a List<ImpactPayload>. This is strange since ImpactPayload comes from org.sonarsource.sonarlint.core.serverapi.push.parsing.common, which is related to the payload of server events. We should maybe have a separate type for the storage. Maybe an improvement for later

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 agree it can be improvement for later. Created this ticket to track: https://sonarsource.atlassian.net/browse/SLCORE-1021

@eray-felek-sonarsource eray-felek-sonarsource force-pushed the feature/ef/SLCORE-999-store-custom-impacts branch from 9de7393 to 576c7a1 Compare October 30, 2024 13:40
Copy link

@eray-felek-sonarsource eray-felek-sonarsource merged commit 1bf8dcf into mmf/SLCORE-959-custom-severities Oct 30, 2024
11 checks passed
@eray-felek-sonarsource eray-felek-sonarsource deleted the feature/ef/SLCORE-999-store-custom-impacts branch October 30, 2024 14:02
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.

2 participants