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

Handle ambigous profile/group parameters #4377

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

noraz31
Copy link
Collaborator

@noraz31 noraz31 commented Oct 29, 2024

Introduce the -e group: option, to improve
handling of ambigous parameters
(such as "security" etc.)

@noraz31 noraz31 force-pushed the handle_ambigous_parameters branch 4 times, most recently from 23f9a04 to 92cbeaa Compare October 30, 2024 12:55
@noraz31 noraz31 marked this pull request as draft November 4, 2024 11:57
Introduce the -e group: option, to improve
handling of ambigous parameters
(such as "security" etc.)
@noraz31 noraz31 marked this pull request as ready for review November 4, 2024 14:55
@dkrupp dkrupp requested a review from cservakt November 5, 2024 14:19
@dkrupp dkrupp added this to the release 6.25.0 milestone Nov 5, 2024
Copy link
Collaborator

@vodorok vodorok left a comment

Choose a reason for hiding this comment

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

I feel like this change is a backward incompatible change, and would requires another reviewers opinion.
@dkrupp what is your opinion?

self.set_checker_enabled(checker, enabled)
LOG.error(templ.substitute(entity="profile",
identifier=identifier))
sys.exit(1)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is exit status 1 the proper exit code in case of configuration errors?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Same applies 10 lines below

@@ -210,23 +212,40 @@ def initialize_checkers(self,
profiles = checker_labels.get_description('profile')
guidelines = checker_labels.occurring_values('guideline')

templ = Template("The ${entity} name '${identifier}' conflicts with a "
"checker(-group) name. Please use -e ${entity}: to "
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should be very specific about what conflicts with what. So if you can provide an exact error message instead of ...with a checker(-group)... then please be specific.

@@ -23,7 +23,8 @@ def available(ordered_checkers, available_checkers):
if checker_name.startswith('profile:') or \
checker_name.startswith('guideline:') or \
checker_name.startswith('severity:') or \
checker_name.startswith('sei-cert:'):
checker_name.startswith('sei-cert:') or \
checker_name.startswith('group:'):
Copy link
Collaborator

Choose a reason for hiding this comment

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

If you plan to introduce a new checker enable prefix, please add this to the documentation and help of CodeChecker analyze and CodeChecker check.

@@ -216,10 +225,15 @@ def f(checks, checkers):

# Enable "security" profile checkers without "profile:" prefix.
cfg_handler = ClangSA.construct_config_handler(args)
cfg_handler.initialize_checkers(checkers,
[('security', True)])
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why are you replacing the security prefix? enabling with sensitive?

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.

3 participants