PYSCAN-41 Implement and test handling of duplicate arguments

[guillaume-dequenne-sonarsource]

In the sonar-scanner-cli, CLI arguments take precedence over the ones located in sonar-project.properties files. Also when duplicated CLI arguments are provided, the last one to be defined takes precedence over the older ones.

Because arguments derived from pyproject.toml files are passed from the wrapper to the sonar-scanner-cli as command line arguments, they will naturally take precedence over the ones in sonar-project.properties. Since only arguments under the tool.sonar key are considered, this should provide a good user experience.

To ensure CLI arguments to the wrapper keep their precedence over pyproject.toml ones, they are passed last to the sonar-scanner-cli.

We might want to revisit this logic if we decide to derive analysis properties from other elements from pyproject.toml files that are not under the tool.sonar key (e.g: project name, etc...), as we don't want to force users to provide CLI arguments to override these.

[sonarcloud]

Quality Gate passed

Kudos, no new issues were introduced!

0 New issues
0 Security Hotspots
100.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

[jdodinh]

LGTM, but I suggested a refactor. LMKWYT.

[jdodinh]

I would like to mention something I meant to mention in Maksim's PR, but that I didn't have a chance to. It is a bit unrelated to this PR, however the fix is small, therefore I think it might still be worth to incorporate this.

Currently, we have that sys.argv[1:] is being parsed in _read_wrapper_arguments, and relevant argument are fetched there. Therefore I don't really see why self.scan_arguments needs to be extended by the entire sys.argv[1:], because I think that we would be passing irrelevant argument to the scanner (such as the .toml, or the debug property, which I believe are only relevant for the wrapper).

Therefore as a refactor I suggest making use of the fact that ArgumentParser.parse_known_args's second return value is all the arguments that couldn't be parsed, and therefore the only ones necessary to be passed down to the cli. I suggest the following:

    def setup(self) -> None:
        """This is executed when run from the command line"""
        self. wrapper_arguments, remaining_args = self._read_wrapper_arguments()
        ApplicationLogger.set_debug(self.is_debug())
        self.scan_arguments = self._read_toml_args()
        # If duplicate properties are provided, newer values will override older ones.
        # We therefore read CLI arguments last so that they have priority over toml configuration.
        self.scan_arguments.extend(remaining_args)

    def _read_wrapper_arguments(self):
        argument_parser = argparse.ArgumentParser()
        argument_parser.add_argument("-toml.path", "-Dtoml.path", "--toml.path", dest="toml_path")
        argument_parser.add_argument("-project.home", "-Dproject.home", "--project.home", dest="project_home")
        argument_parser.add_argument("-X", action="store_true", dest="debug")
        return argument_parser.parse_known_args(args=sys.argv[1:])

[guillaume-dequenne-sonarsource]

This is a very good point.

However we should remember some arguments are actually used by both the wrapper and the sonar-scanner-cli, for instance the project.home or the -X flag. Also we should make sure that we don't risk filtering out properties that may one day become relevant for sonar-scanner-cli (for instance, what if toml files were eventually supported natively by the sonar-scanner-cli?).

As discussed, I created PYSCAN-43 to ensure we are at least intentional on this point.

[jdodinh]

Related to my comment about refactoring, I don't really see why the toml path needs to be passed down to the scanner. It seems only relevant to the wrapper, and therefore I think some of the test cases might need to be changed to reflect this logic.

[jdodinh]

Ditto, and same for line 173

[jdodinh]

Ditto