-
Notifications
You must be signed in to change notification settings - Fork 32
refactor: rename automatic_linkage module and its internal functions #892
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
Open
adekoder
wants to merge
1
commit into
NixOS:main
Choose a base branch
from
adekoder:name-refactor-for-automatic-linkage-module
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,5 @@ | ||
| import shared.listeners.nix_channels # noqa | ||
| import shared.listeners.nix_evaluation # noqa | ||
| import shared.listeners.automatic_linkage # noqa | ||
| import shared.listeners.cve_derivation_matcher # noqa | ||
| import shared.listeners.cache_suggestions # noqa | ||
| import shared.listeners.notify_users # noqa |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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.
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'm so sorry, the naming is a mess in this code base. We're still cleaning up the prototype-era fallout. @florentc do we agree that we've firmly converged on calling the automatic matchings "suggestions"?
Uh oh!
There was an error while loading. Please reload this page.
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.
but isn't suggestion in the context of how the frontend treat matches ?
Uh oh!
There was an error while loading. Please reload this page.
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.
A(n automatically generated) suggestion links CVEs and (currently) derivations (we want that to be packages #617), we also call these links matches. We call the process "linking" or "matching". Ideally the frontend would directly visualise that data model.
The code is still all over the place, sometimes they are called "proposals" or "matches" or "links". I think we should stick with "suggestions", but "matches" is not wrong either (but would require starting to think about redirects from deprecated URL patterns).
Uh oh!
There was an error while loading. Please reload this page.
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'm biased as I spend most of my time in
webviewrather thanshared. Inwebview, clearly, "suggestion" is the term that's used to refer to aCVEDerivationClusterProposaland its cached information. Stuff that is called suggestion usually has typeCVEDerivationClusterProposal. It used to also refer to only the cached data. I tried to bring some consistency these last few months by always calling suggeston theCVEDerivationClusterProposaland access the cached field and the payload explicitly. There might still be some leftover.So yeah, in
webviewat least, it would totally make sense to renameCVEDerivationClusterProposaltoSuggestion.In
sharedhowever I can't say with as much confidence. In the backend at the lower level, it's true that calling it "match" gives a better intuition about what it is. But this object has fields like "comment" and "status" which clearly go beyond the description of a match. Should we maybe have aMatchtable and andSuggestiontable, each suggestion having a match (the CVE and Derivations) and adding fields describing its current state (status, comment, PackageEdits, MaintainersEdits, etc) in the suggestion workflow?Sorry I'm not helping, just brainstorming too
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.
Separating matches and "the thing that people edit" (I'm inclined to call that a "draft" as it was before in the UI, even at the risk of leaving the question "but draft of what?" or possibly "draft issue"). Then this draft would be part of a published issue, which tacks on even more data.
Most importantly this would be the first step towards having issues with multiple matches!
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.
Indeed! The draft could be linked to several matches, we could have an "add another match to the draft" UI thing, and the issue would be linked to 1 draft.
In fact, maybe issue and draft could be merged. That would clear any naming weirdness.
Match: a CVE the system matched to some derivations (and the associated packages/severity/maintainers)
Issue: A thing the user edits. Associated to 1 or several matches. Which could be in various status: untriaged, dismissed, draft, and published. When in "dismissed" or "published" the issue is frozen like suggestions are now (no edits).
Publication: What becomes attached to an issue once in "published" status. Publication would have a github link, and additional data such as open/closed, tags, and whatever we want to display (on sec tracker) is happening on GitHub about this issue.
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.
Sounds good. We can pull out
cveandderivationsinto aMatchmodel. We just need to think about what happens with the ignored/additional items, since right now they live on what would end up asIssueand thus apply to all of the matches. Is that what we want?Uh oh!
There was an error while loading. Please reload this page.
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.
For the record, here is what would happen with a basic approach to the problem where
Matchare immutable andIssueproposes to apply...Editsto packages and maintainers (and references).Each
Matchhas a set of packages (in fact derivations but viewed grouped by attribute name as packages) and their associated maintainers (and references)An
Issuemay be associated to several matches.An
Issuemay have 0, 1 or severalPackageEdits,MaintainersEdit, andReferencesEditThe current state (and what's displayed) of the lists of packages/maintainers/references of an
Issuewould be the union of all the packages/maintainers/references of all its associated matches, on which additions and removals are applied by the overlayPackageEdit/MaintainersEdit/ReferencesEditof thatIssue.This is, IMO, the straightforward first approach we can take.