Fix for #92 Display warning about uninstalled plugins in the Administration section of SonarQube #105
Conversation
|
|
| static final String ISSUES_SEVERITY_KEY = "sonar.Devon.preview.issuesSeverity"; | ||
|
|
||
| private static final String QUALINSIGHT = "qualinsight-plugins-sonarqube-smell-plugin"; | ||
| private static final String PMD = "sonar-pmd-plugin"; |
There was a problem hiding this comment.
Please make sure to use proper devonfw formatting. You can either do it manually by copying the format of code that is already on the master branch, or simply import devonfw format settings into your IDE.
| static List<String> FORBIDDEN_REPO_KEYS = new ArrayList<>(); | ||
| private File pluginDirectory; | ||
| private List<String> pluginList; | ||
| private List<String> requiredPlugins = Arrays.asList("qualinsight-plugins-sonarqube-smell-plugin", |
There was a problem hiding this comment.
Putting the required plugins inside a list is fine, but then we wouldn't need them double. As it is now, we have individual strings for all the plugins we need, plus this list. One of them should be enough
| .subCategory("").type(PropertyType.TEXT).defaultValue(getMissingPlugins()) | ||
| .build()); | ||
| disableRepoKeys(); | ||
| disableRepoKeys(); |
There was a problem hiding this comment.
Why is this method called twice here?
| } | ||
| private String getMissingPlugins() { | ||
| StringBuilder missingPlugins = new StringBuilder(); | ||
| missingPlugins.append("Please install plugins listed below: \n\n"); |
There was a problem hiding this comment.
The warning is good like this, but we should only display it if there really is a plugin missing. As it is now, this warning will also be shown when all the plugins are installed. A solution could be to surround the 'context.addExtension()' call inside 'define()' with an if-condition, checking if one of the required plugins is missing.
| if (missingPlugins.length() != 0) { | ||
| missingPlugins.insert(0, "Please install plugins listed below: \n\n"); | ||
| } else { | ||
| missingPlugins.append("All plugins installed properly"); |
There was a problem hiding this comment.
Could you add punctuation after this information?
There was a problem hiding this comment.
Thanks for this PR. @lmarniazman would you be able to adopt this PR or @bartoszbicz96 are you still available for improving this PR? Otherwise we could merge it as is and you could afterwards create a new PR with the suggested improvements.
| @@ -96,15 +57,13 @@ public void define(Context context) { | |||
| break; | |||
| } | |||
| } | |||
|
|
|||
| if (!(FORBIDDEN_REPO_KEYS.contains(repoKey) || repoKey == null || ruleKey == null)) { | |||
| if (!(SonarDevon4jPlugin.FORBIDDEN_REPO_KEYS.contains(repoKey) || repoKey == null || ruleKey == null)) { | |||
There was a problem hiding this comment.
I am fine with this PR and its changes but whilst reviewing, I noticed that SonarDevon4jPlugin.FORBIDDEN_REPO_KEYS is odd: Members in Java are either a constant with a NAME_IN_UPPER_CASE_SYNTAX or they are mutable and have a regularCamlCaseName. We should therefore update SonarDevon4jPlugin and change FORBIDDEN_REPO_KEYS to final. Furhter we are making assumtions about initialization order here what is very dangerous and might even change in future SonarQube versions. In order to at least detect such problem, it would IMHO be much more clean to change FORBIDDEN_REPO_KEYS also to private and create a static getter method for it. This method could then even check if the initialization has taken place already (e.g. by another static flag) and otherwise log an error or throw an exception.
| @@ -26,5 +58,64 @@ public void define(Context context) { | |||
| .type(PropertyType.TEXT) | |||
| .defaultValue("{\"architecture\":{\"components\":[\n{\"name\":\"component1\",\\\"dependencies\\\":[]}}\n]}}") | |||
| .build()); | |||
| context.addExtension(PropertyDefinition.builder(DISABLED).name("Warning") | |||
| .description("Missing plugins for full initialization of devonfw quality profile").category("devonfw") | |||
| .subCategory("").type(PropertyType.TEXT).defaultValue(getMissingPlugins()).build()); | |||
There was a problem hiding this comment.
if getMissingPlugins() is empty we would still get this warning? This is bad UX. We could simply add an if-statement around this code.
There was a problem hiding this comment.
There already is an if-statement in place. If the number of missing plugins is 0, the statement "All plugins installed properly" is displayed. If this was the only critical comment, I suggest we could merge this.
…page_in_the_Administration_section_SonarQube
|
Replaced by and continued in PR #115 |
#92
-creating a new field with warning about uninstalled plugins
-new method checking plugins (getMissingPlugins)