-
Notifications
You must be signed in to change notification settings - Fork 30
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
RULEAPI-826 Compute rule / product mapping #4575
base: master
Are you sure you want to change the base?
Conversation
rspec-tools/rspec_tools/coverage.py
Outdated
def collect_coverage_per_product(rules_dir): | ||
git_repo = checkout_repo('sonar-enterprise') | ||
bundle_map = get_packaged_plugins(git_repo) | ||
tags = git_repo.tags | ||
tags.sort(key = lambda t: t.commit.committed_date) | ||
versions = [tag.name for tag in tags if is_interesting_version(tag.name)] | ||
plugin_versions = {} | ||
for version in versions: | ||
plugin_versions[version] = get_plugin_versions(git_repo, version) | ||
build_rule_per_product(rules_dir, bundle_map, plugin_versions) |
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.
-
I'm concerned we do not sort things consistently with
collect_coverage_for_all_versions
(committed date vs tag).
Maybe the rest of the code handles this nicely, in which case I would remove this sort and test the implementation with a shuffle to make sure it's resilient. -
I also think we can have a lightweight test here that covers scenarios like:
- SQS 2025.1 is released with plugin X.1 and thus S1
- (Assume 2025.1 is an LTA)
- SQS 2025.2 is released with plugin X.2 and drops S1
- bugfix SQS 2025.1.1 is released still with X.1 (or X.1.1) and with S1
or this one:
- SQS 2025.1 is released with plugin X.1 and no S1
- (Assume 2025.1 is an LTA)
- SQS 2025.2 is released with plugin X.2 and introduces S1
- bugfix SQS 2025.1.1 is released still with X.1 (or X.1.1) and thus without S1
In terms of test implementation, I would suggest:
- pass the
git_repo
as a parameter - create a fake
sonar-entreprise
for a couple of scenarios -- we just need tags & specific files. - create a fake plugin coverage JSON file for those scenarios
- a big of pytest magic should be enough -- we may not even need to mock/patch anything
I appreciate we do not want complicated unit tests for each function (although it would be nice), but we can strike a balance between having nothing and something that cover the main corner cases from end-to-end.
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.
Tests will be covered by RULEAPI-828
product_per_rule_per_lang['SonarQube Server'] = { | ||
'minimal-edition': EDITIONS[0], | ||
'since-version': lowest_server_version(plugin_versions, plugin, version) | ||
} |
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.
We'll get a hard error (thanks to the sys.exit
) if a rule is released for the first time in a Community Build (i.e., it is not yet released in a SQS version).
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.
Oh, good point!
One cool thing we could do is handle it gracefully and return something like "Coming soon" or "Next release" in that case.
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.
I would suggest keeping this as-is for now and fixing this with RULEAPI-830 and a proper test (i.e., after RULEAPI-828) to focus on the essentials right now.
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.
I made the change because it is a scenario that is supposed to happen very quickly after the release of the first LTA, as SQ Community Builds have a shorter cycle than SQ Enterprise.
I also realized that there was a bug in how we handled "master" branches, which once fixed, would trigger the same exception.
rspec-tools/rspec_tools/coverage.py
Outdated
@@ -49,6 +49,12 @@ | |||
|
|||
RULES_FILENAME = 'covered_rules.json' | |||
|
|||
DEPENDENCY_RE = re.compile(r'''\bdependency\s+['"](?:com|org)\.sonarsource\.[\w.-]+:(?P<plugin_name>[\w-]+):(?P<version>\d+(\.\d+)+)['"]''') |
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.
Right now, in the code, we say major >= 8
however this regex does not match for all plugins.
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.
9.x looks compatible though: https://github.com/SonarSource/sonar-enterprise/blob/ce743843018827cb30c05735e7971523de34bb48/build.gradle#L171-L209
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.
Starting 10.0 we have an entry that does not match the regex but we should not care about this one so that's fine: https://github.com/SonarSource/sonar-enterprise/blob/75dae6a655f49ac78b0a786f6c19b15085561af9/build.gradle#L218
10.1 introduced a second one, but again we should be good. https://github.com/SonarSource/sonar-enterprise/blob/53c01c35c264c7e3d76cf5fb955de406f36b115e/build.gradle#L254-L255
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.
I've pushed 3b74531 to:
- support
>=9
only - detect missing versions if variables are re-introduced for plugins in the future.
If we need older coverage, we can accommodate that in a later PR. @frederic-tingaud-sonarsource can you validate this is OK for now?
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.
Thanks for catching these! It looks good to me.
Loosen regex to detect unexpectedly missing version in the future.
Quality Gate failed for 'rspec-tools'Failed conditions |
Quality Gate passed for 'rspec-frontend'Issues Measures |
Your changes look good to me. Feel free to merge. |
@@ -300,7 +300,7 @@ def lowest_version(plugin_versions, plugin, version, skip_suffix): | |||
pvv = plugin_versions[t][plugin] | |||
if comparable_version(pvv) >= comparable_version(version): | |||
return t | |||
sys.exit(f'failed finding the oldest version of {plugin} containing {version} from {plugin_versions}') | |||
return "Coming soon" |
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.
I'm not sure we want a "coming soon" fake version. It would be better to not give such version (i.e., make both SonarQube Community Build
and SonarQube Server
optional).
See also my comment here: https://sonarsource.atlassian.net/browse/SONARSITE-2248?focusedCommentId=709499
# The rule has an "until", therefore it does not exist anymore | ||
# and should not appear in the product mapping. | ||
continue | ||
target_repo, version = since.split(' ') |
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.
We should probably skip the master
version here since it's not available in the product anyway and we do not look at master
versions of sonar-enterprise.
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.
I had a look at the data we produce and there is something fishy. I'll hold off merging this because I don't want the script to cause trouble during the holidays when we are not there -- it would not be a nice Xmas gift.
Looking at S100 (very old rule), we say:
- for C/C++/ObjC: introduced in 9.0 Dev edition ✅
- for Apex: enterprise edition (probably correct), introduced in 10.7: does not match reality SQS report it being there since Nov 02, 2018.
- for C#: Dev edition (sounds correct) but introduced in SQS 10.8 / sqcb-24.12 but SQ reports the rule being introduced in 2015...
I don't know where we do things incorrectly...
No description provided.