Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Scorecard Integration #1294
base: main
Are you sure you want to change the base?
Scorecard Integration #1294
Changes from 41 commits
173db43
272b99c
944cee2
bc445c1
f241b3b
605a5cf
3833dca
37380c8
5502b5f
2aeb2a4
103fca0
952e6a6
3ba3db5
2f2b846
39a056d
e5f3e7a
c2f5c4d
6ee5b07
760afc2
0dbc92f
d652f42
aa154c0
37ad73a
563991b
4632dfc
923c834
94bfcd0
d129f73
24c7be0
259f004
ccd75ae
9d80ef1
29da290
9d72734
301122e
5812d97
df50416
88a43dc
fc4945e
c00b3b3
b97ff7a
496945b
f86d5bb
36e955a
90c113a
43886a3
7c134c7
3d4d6ea
f4ed4b5
b9229a5
f80f111
ee2ea14
bd5b9b3
2b4629e
99ee48d
80153f8
77efae0
113a557
64e17fe
027fe2d
7f43f9e
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Follow the conventions used across the existing Models:
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.
Can a DiscoveredPackageScore instance really exists without a DiscoveredPackage FK defined?
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.
Not sure why this is required here? We are not doing multiple database updates that could crash and needs to be handled?
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.
Since we are updating the checks and score table at one go for that reason, I have kept it atomic
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.
@404-geek Could you provide an example that shows why atomic() is useful here?
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.
@404-geek You haven't address the question above yet ;)
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.
Is there cases where we don't have dates? Can you show some examples?
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.
Are there so many checks where this would make a difference in time taken to save these?
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.
Why is this required? Do you show this anywhere?
I don't think showing just the score/uuid is helpful at all. We need the repo + score anyway if you're logging this. But you should get the values from the objects directly where you log it.
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 just kept it like that
Do you have anything in mind to return if someone calls the instance directly.
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.
Follow the conventions used across the existing Models:
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.
Are those really optional fields?
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.
Why not create in directly and have defaults? Doesn't seem like we are doing much here
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.
@AyanSinhaMahapatra thanks for pointing out.
I have updated it to create the object directly
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.
fetch_packages_scorecode_info
would be better.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.
That line seems unnecessary.
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.
This should not be loaded in the module init but as needed in the test function context.