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

Experimental --scoring flag for scorecards metrics #1155

Merged
merged 2 commits into from
Aug 22, 2022

Conversation

mayaCostantini
Copy link
Contributor

Related Issues and Dependencies

Related to thoth-station/core#434 and #1149

This introduces a breaking change

  • No

This should yield a new module release

  • Yes

This Pull Request implements

Users can pass the --scoring flag to the thamos advise command to get a summary of metrics about the quality of their dependencies as described by Security Scorecards. The next step would be to aggregate metrics about packages present in Thoth's knowledge base to be able to compare the user's dependencies quality to the average dependency quality based on those metrics.

Screenshot from 2022-07-30 12-32-20

@sesheta sesheta added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jul 30, 2022
@mayaCostantini mayaCostantini force-pushed the experimental-metrics branch 2 times, most recently from 407d08a to 4c2f5f1 Compare July 30, 2022 10:59
@mayaCostantini
Copy link
Contributor Author

Related to #1148

@mayaCostantini
Copy link
Contributor Author

@harshad16 @KPostOffice ready for review 👍

@goern
Copy link
Member

goern commented Aug 9, 2022

/lgtm

@sesheta sesheta added the lgtm Indicates that a PR is ready to be merged. label Aug 9, 2022
Copy link
Member

@harshad16 harshad16 left a comment

Choose a reason for hiding this comment

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

lgtm
just one suggestion.

thamos/cli.py Outdated
Comment on lines 349 to 377
if "Project " in message:
message = message.replace("Project ", "projects ")

if " is " in message:
message = message.replace(" is ", " are ")

if " does " in message:
message = message.replace(" does ", " do ")

if " has " in message:
message = message.replace(" has ", " have ")

if " runs " in message:
message = message.replace(" runs ", " run ")

if " requires " in message:
message = message.replace(" requires ", " require ")

if " uses " in message:
message = message.replace(" uses ", " use ")

if " follows " in message:
message = message.replace(" follows ", " follow ")

if " signs " in message:
message = message.replace(" signs ", " sign ")

if " NOT " in message:
message = message.replace(" NOT ", " not ")
Copy link
Member

Choose a reason for hiding this comment

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

can share what functionality is this for, it this to fix the grammer of the sentence ?

also how about using a dict with these words and replacing them with a function looping over dict and message? is there any significant value to use if conditions here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I implemented this (dirty) workaround to correct the message grammar while we wait for more prescriptions to be generated using tags. When most prescriptions will contain metadata about the type of Scorecard they are based on in their tag, then we could remove this and leave only the logic that uses a dictionary I also implemented in this PR. Does this sound like a good temporary solution to you?

Copy link
Member

Choose a reason for hiding this comment

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

ack, i think this clears my thoughts.
As we would like to have these grammar change till we wait on prescriptions to be generated using tags.
how about, make the grammer fix words into a dict solution, so that if we found something else needed to be correct just would need to be added to a dict instead of a new if statement.
just for easy of updates.

Example:

def fix_message(message, replace_words):
   for k,v in replace_words.items():
        message = message.replace(k,v)
   return message
   
replace_words = {"Projects":"project", "is":"are",...}
msg= fix_message(mesage,replace_wrods)

Wdyt? i dont feel strongly about this, if you say it's temp, we can go with it.
Its just that we have too many if statement doing the same task.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that sounds better to me, I will make this change 👍

@sesheta sesheta removed the lgtm Indicates that a PR is ready to be merged. label Aug 22, 2022
@mayaCostantini
Copy link
Contributor Author

@harshad16 ready for review 👍

@sesheta
Copy link
Member

sesheta commented Aug 22, 2022

New changes are detected. LGTM label has been removed.

@goern
Copy link
Member

goern commented Aug 22, 2022

/approve

@sesheta
Copy link
Member

sesheta commented Aug 22, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: goern

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@sesheta sesheta added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 22, 2022
@goern
Copy link
Member

goern commented Aug 22, 2022

@mayaCostantini could you do a tiny asciinema demo for the new output?

@sesheta sesheta merged commit 1ba3eca into thoth-station:master Aug 22, 2022
@mayaCostantini
Copy link
Contributor Author

@goern Sure

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants