RULEAPI-654: Clarify the rule creation process (#115)
This commit is contained in:
parent
dd6fb41117
commit
520573b838
56
README.adoc
56
README.adoc
@ -36,7 +36,21 @@ WARNING: Unlike the Search Page, the GitHub search across the PRs for unimplemen
|
|||||||
|
|
||||||
Before, the Languages Team used Jira to host both implemented and unimplemented rules. This is why the `rules` directory contains both too.
|
Before, the Languages Team used Jira to host both implemented and unimplemented rules. This is why the `rules` directory contains both too.
|
||||||
|
|
||||||
However, one of the reasons we are migrating to a git repository is that we want to have a clean process and history for rule creation and modification. Thus every newly created rule or modification of rule should follow this workflow:
|
However, one of the reasons we are migrating to a git repository is that we want to have a clean process and history for rule creation and modification.
|
||||||
|
|
||||||
|
In particular, the main branch aims at representing what will be integrated in the next version of the analyzers, i.e. what will be part of the next releases.
|
||||||
|
|
||||||
|
Thus every newly created rule or modification of rule should follow these steps:
|
||||||
|
|
||||||
|
. Create a pull request adding or modifying a rule
|
||||||
|
. Ask for a review
|
||||||
|
. Create an implementation ticket
|
||||||
|
. Implement the new rule or the change in the existing rule
|
||||||
|
. Merge the RSPEC PR as soon as the implementation is ready
|
||||||
|
. Fetch the updated metadata with `rule-api`
|
||||||
|
. Merge the implementation PR
|
||||||
|
|
||||||
|
A <<multi-language-rule-creation>> is somewhat more involved.
|
||||||
|
|
||||||
=== 1. Create a pull request
|
=== 1. Create a pull request
|
||||||
|
|
||||||
@ -156,9 +170,15 @@ Implementation ticket: SonarSource/sonar-dotnet/issues/1234 (for a sonar-dotnet
|
|||||||
|
|
||||||
=== 4. Implement the rule
|
=== 4. Implement the rule
|
||||||
|
|
||||||
Implement the rule or the modification as usual. +
|
Implement the rule or the modification as usual.
|
||||||
Merge the RSPEC pull request before updating the metadata of the analyzer otherwise `rule-api` will not work. +
|
|
||||||
And finally, merge the rule implementation in your analyzer repository.
|
Only once the implementation is complete, but before it is merged on the analyzer side, merge the RSPEC PR.
|
||||||
|
The RSPEC PR has to be merged before the implementation PR to enable `rule-api` to fetch the correct metadata in the analyzer.
|
||||||
|
The RSPEC PR has to be merged as close as possible to the merge of the implementation PR to shorten the time span of the inconsistency in the rule status
|
||||||
|
("active" in the RSPEC metadata, and not implemented on the analyzer side).
|
||||||
|
|
||||||
|
Finally, merge the rule implementation in your analyzer repository.
|
||||||
|
|
||||||
|
|
||||||
==== Generate/Update rule metadata for the analyzer
|
==== Generate/Update rule metadata for the analyzer
|
||||||
|
|
||||||
@ -178,6 +198,34 @@ you have to run `update` immediately after,
|
|||||||
because rule-api relies on the files in the directory to determine the covered set of rules
|
because rule-api relies on the files in the directory to determine the covered set of rules
|
||||||
when generating the deprecation notes for superseded rules.
|
when generating the deprecation notes for superseded rules.
|
||||||
|
|
||||||
|
=== Multi-Language Rule Creation
|
||||||
|
Multi-language rule creation is more involved than the default process because it involves multiple roles that typically do not coincide.
|
||||||
|
It is infeasible to synchronize the implementation of the rule for all the languages it covers.
|
||||||
|
|
||||||
|
The special metadata field `"extra"."coveredLanguages"` enables asynchrous implementation in multiple analyzers.
|
||||||
|
|
||||||
|
`"extra"."coveredLanguages"` contains the languages the rule is implemented for.
|
||||||
|
|
||||||
|
The workflow below makes sure that all rules on the main branch are implemented for all languages they are specified for or for all languages listed in `"coveredLanguages"`.
|
||||||
|
|
||||||
|
. An RSPECator creates a PR and specifies the multi-language rule.
|
||||||
|
* The RSPECator asks for a review for the PR.
|
||||||
|
* The RSPECator does not merge the PR, even after the review is done.
|
||||||
|
* The rule metadata.json contains an empty `"extra": {"coveredLanguages": []}` field.
|
||||||
|
. The RSPECator opens implementation tickets for all the targeted languages.
|
||||||
|
. An Ada analyzer developer Alice implements the rule first. Alice prepares the PR with the implementation.
|
||||||
|
. As soon as the implementation of the rule is ready for Ada analyzer, Alice merges both PRs:
|
||||||
|
.. Alice adds `"Ada"` to `"coveredLanguages"` in the RSPEC PR (`"extra": {"coveredLanguages": ["Ada"]}`).
|
||||||
|
.. Alice merges the RSPEC PR.
|
||||||
|
.. Alice fetches the rule metadata with `rule-api` into Ada analyzer.
|
||||||
|
.. Alice merges the rule implementation in Ada analyzer.
|
||||||
|
. A Cobol analyzer developer Bob implements the rule some time later. Bob prepares the PR with the implementation.
|
||||||
|
. Bob opens a new RSPEC PR "Modify rule S1234: Add Cobol support" to add `"Cobol"` to `"coveredLangauges"` (`"extra": {"coveredLanguages": ["Ada", "Cobol"]}`).
|
||||||
|
. As soon as the Cobol implementation is ready, Bob merges both PRs:
|
||||||
|
.. Bob merges his RSPEC PR.
|
||||||
|
.. Bob fetches the rule metadata with `rule-api` into Cobol analyzer.
|
||||||
|
.. Bob merges the rule implementation in Cobol analyzer.
|
||||||
|
|
||||||
== Comment a rule
|
== Comment a rule
|
||||||
|
|
||||||
Comments and links that were created on Jira have been gathered in a `comments-and-links.adoc` file for each concerned rule. +
|
Comments and links that were created on Jira have been gathered in a `comments-and-links.adoc` file for each concerned rule. +
|
||||||
|
@ -8,9 +8,12 @@
|
|||||||
},
|
},
|
||||||
"tags": [
|
"tags": [
|
||||||
],
|
],
|
||||||
|
"extra": [
|
||||||
|
"coveredLanguages": []
|
||||||
|
],
|
||||||
"defaultSeverity": "Major",
|
"defaultSeverity": "Major",
|
||||||
"ruleSpecification": "RSPEC-${RSPEC_ID}",
|
"ruleSpecification": "RSPEC-${RSPEC_ID}",
|
||||||
"sqKey": "S${RSPEC_ID}",
|
"sqKey": "S${RSPEC_ID}",
|
||||||
"scope": "All",
|
"scope": "All",
|
||||||
"qualityProfiles": ["Sonar way"]
|
"qualityProfiles": ["Sonar way"]
|
||||||
}
|
}
|
||||||
|
@ -3,7 +3,7 @@
|
|||||||
"title": "Rule Implementation",
|
"title": "Rule Implementation",
|
||||||
"type": "object",
|
"type": "object",
|
||||||
"description": "we must have one of these files for each implemented rule",
|
"description": "we must have one of these files for each implemented rule",
|
||||||
"additionalProperties": true,
|
"additionalProperties": false,
|
||||||
"properties": {
|
"properties": {
|
||||||
"title": {
|
"title": {
|
||||||
"type": "string"
|
"type": "string"
|
||||||
@ -16,6 +16,28 @@
|
|||||||
"type": "string",
|
"type": "string",
|
||||||
"enum": ["beta","ready","deprecated","superseded", "closed"]
|
"enum": ["beta","ready","deprecated","superseded", "closed"]
|
||||||
},
|
},
|
||||||
|
"extra": {
|
||||||
|
"type": "object",
|
||||||
|
"properties": {
|
||||||
|
"additionalProperties": false,
|
||||||
|
"coveredLanguages": {
|
||||||
|
"type": "array",
|
||||||
|
"items": {
|
||||||
|
"type": "string",
|
||||||
|
"uniqueItems": true
|
||||||
|
},
|
||||||
|
"description": "The languages that already implement this rule"
|
||||||
|
},
|
||||||
|
"replacementRules": {
|
||||||
|
"type": "array",
|
||||||
|
"items": {
|
||||||
|
"type": "string",
|
||||||
|
"uniqueItems": true
|
||||||
|
},
|
||||||
|
"description": "The rule ids that replace this rule"
|
||||||
|
}
|
||||||
|
}
|
||||||
|
},
|
||||||
"remediation": {
|
"remediation": {
|
||||||
"type": "object",
|
"type": "object",
|
||||||
"oneOf": [
|
"oneOf": [
|
||||||
@ -160,6 +182,11 @@
|
|||||||
"pattern": "MSTG-[A-Z]+-[0-9]+"
|
"pattern": "MSTG-[A-Z]+-[0-9]+"
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
},
|
||||||
|
"defaultQualityProfiles": {
|
||||||
|
"type": "array",
|
||||||
|
"items": { "type": "string"},
|
||||||
|
"uniqueItems": true
|
||||||
}
|
}
|
||||||
},
|
},
|
||||||
"if": {
|
"if": {
|
||||||
|
@ -36,9 +36,10 @@ def test_invalid_remediation_fails_validation(rule_language: LanguageSpecificRul
|
|||||||
validate_metadata(rule_language)
|
validate_metadata(rule_language)
|
||||||
|
|
||||||
|
|
||||||
def test_adding_properties_pass_validation(rule_language: LanguageSpecificRule):
|
def test_adding_properties_fails_validation(rule_language: LanguageSpecificRule):
|
||||||
metadata = deepcopy(rule_language.metadata)
|
metadata = deepcopy(rule_language.metadata)
|
||||||
metadata['unknown'] = 42
|
metadata['unknown'] = 42
|
||||||
with patch.object(LanguageSpecificRule, 'metadata', new_callable=PropertyMock) as mock:
|
with pytest.raises(RuleValidationError, match=fr'^Rule {rule_language.id} has invalid metadata'):
|
||||||
mock.return_value = metadata
|
with patch.object(LanguageSpecificRule, 'metadata', new_callable=PropertyMock) as mock:
|
||||||
validate_metadata(rule_language)
|
mock.return_value = metadata
|
||||||
|
validate_metadata(rule_language)
|
||||||
|
@ -13,5 +13,5 @@
|
|||||||
"ruleSpecification": "RSPEC-6294",
|
"ruleSpecification": "RSPEC-6294",
|
||||||
"sqKey": "S6294",
|
"sqKey": "S6294",
|
||||||
"scope": "Main",
|
"scope": "Main",
|
||||||
"qualityProfiles": ["Sonar way"]
|
"defaultQualityProfiles": ["Sonar way"]
|
||||||
}
|
}
|
||||||
|
@ -13,5 +13,5 @@
|
|||||||
"ruleSpecification": "RSPEC-6295",
|
"ruleSpecification": "RSPEC-6295",
|
||||||
"sqKey": "S6295",
|
"sqKey": "S6295",
|
||||||
"scope": "Main",
|
"scope": "Main",
|
||||||
"qualityProfiles": ["Sonar way"]
|
"defaultQualityProfiles": ["Sonar way"]
|
||||||
}
|
}
|
||||||
|
Loading…
x
Reference in New Issue
Block a user