Feature/382 add nox task for dependencies update #393
Conversation
…ith docstrings - Alter & add missing unit tests
ArBridgeman
temporarily deployed
to
manual-approval
GitHub Actions
Inactive
ArBridgeman
temporarily deployed
to
manual-approval
GitHub Actions
Inactive
ArBridgeman
temporarily deployed
to
manual-approval
GitHub Actions
Inactive
ArBridgeman
temporarily deployed
to
manual-approval
GitHub Actions
Inactive
ArBridgeman
temporarily deployed
to
manual-approval
GitHub Actions
Inactive
|
Do you think you could a short documentation regarding the basic concepts and the approach underlying the implementation (aka. "software design")? |
| before_update_dict = {pkg.name: pkg for pkg in self.before_env} | ||
| after_update_dict = {pkg.name: pkg for pkg in self.after_env} | ||
|
|
||
| def _get_change_str(pkg_name: str) -> str | None: |
There was a problem hiding this comment.
As this is a nested function, I think we can omit the underscore prefix _ - please correct me if I'm wrong.
Also, I remember @Nicoretti being quite allergic regarding the prefix get_ as it often can be removed, and I think I could get acquainted to this opinion.
There was a problem hiding this comment.
Please take a look at the Project keeper: Maybe it's a good idea to maintain three separate lists of updated, added, and removed dependencies and only merge or resp. render them later on when creating a report?
References in PK:
| after_env: set[PackageVersion] = field(default_factory=set) | ||
|
|
||
| @staticmethod | ||
| def _obtain_version_set() -> set[PackageVersion]: |
There was a problem hiding this comment.
How about naming the method _package_versions() or _package_version_set() ?
There was a problem hiding this comment.
If it is a function it should contain a verb
|
|
||
| @staticmethod | ||
| def _obtain_version_set() -> set[PackageVersion]: | ||
| def _get_package_version(line: str) -> PackageVersion: |
There was a problem hiding this comment.
As this is a nested function, I think we can omit the underscore prefix _ - please correct me if I'm wrong.
Also, I remember @Nicoretti being quite allergic regarding the prefix get_ as it often can be removed, and I think I could get acquainted to this opinion.
I propose pkg_version() - short, local, without underscore _ and prefix get_.
Happy to discuss if you prefer a different name.
| return PackageVersion(name=groups[0], version=groups[1]) | ||
|
|
||
| command = ("poetry", "show", "--top-level") | ||
| result = subprocess.run(command, capture_output=True, check=True) |
There was a problem hiding this comment.
You can use argument encoding="utf-8" or text=True to avoid using decode later on.
See https://docs.python.org/3/library/subprocess.html#subprocess.run
| @dataclass(frozen=True) | ||
| class VulnerabilityIssue: | ||
| """ | ||
| Dataclass for a vulnerability to be submitted to GitHub as an issue. |
There was a problem hiding this comment.
Could we maybe rename this class to simply Vulnerability?
And maybe please add some documentation about the relationship and differences between
VulnerabilityIssue and GitHubVulnerabilityIssue.
I have the impression, the former one is generic and maybe comes from pip-audit or dependabot and the latter one maybe does not exist a-priori and is created later on -- or it may exist more or less independently and need to be found and associated with the former.
If this is correct, then please add some documentation on these relationships and lifecycle.
| https://github.com/CVEProject/cve-schema/blob/master/schema/v5.0/CVE_JSON_5.0_schema.json | ||
| Additionally, it is a known that some vulnerabilities may not initially or ever be | ||
| assigned a CVE, which is meant to act as a unique identifier. In such cases, they | ||
| instead have another kind of vulnerability ID associated with them. |
There was a problem hiding this comment.
Could you add a description for each of the attributes and its meaning?
Additional question: where is the another kind of vulnerability ID stored?
| for line in lines: | ||
| obj = json.loads(line) | ||
| obj["references"] = tuple(obj["references"]) | ||
| yield VulnerabilityIssue(**obj) |
There was a problem hiding this comment.
I don't understand these lines. Could we please have a peer session?
| return json.dumps(issue_json) | ||
|
|
||
|
|
||
| class VulnerabilitySource(str, Enum): |
There was a problem hiding this comment.
Please also add a docstring here.
| @@ -396,12 +298,15 @@ def create( | |||
| { "cve": "<cve-id>", "cwe": "<cwe-id>", "description": "<multiline string>", "coordinates": "<string>", "references": ["<url>", "<url>", ...] } | |||
|
|
|||
| Output: | |||
| Links to the created issue(s) | |||
| { "cve": "<cve-id>", "cwe": "<cwe-id>", "description": "<multiline string>", "coordinates": "<string>", "references": ["<url>", "<url>", ...], "issue_url": "<url>" } | |||
There was a problem hiding this comment.
I think this method create() and its docstring could be an excellent reference for many of the other classes, methods and functions. Please consider linking this if appropriate.
| @dataclass | ||
| class PackageVersionTracker: | ||
| """ | ||
| Tracks direct dependencies for package versions before & after updates |
There was a problem hiding this comment.
I am confused by this docstring, do you keep the package versions for direct dependencies, that would make more sense for me
|
Will be going a different way than this. The two tasks would be separated. |
Closes #382