-
-
Notifications
You must be signed in to change notification settings - Fork 234
Modify Elixir Security importer to support package-first mode #1935
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
base: main
Are you sure you want to change the base?
Conversation
* Update Elixir Security importer to filter and process advisories relevant to the purl passed in the constructor * Update Elixir Security v2 importer to filter and process advisories relevant to the purl passed in the constructor * Update Elixir Security importer tests to include testing package-first mode Signed-off-by: Michael Ehab Mikhail <[email protected]>
…importer-package-first Signed-off-by: Michael Ehab Mikhail <[email protected]>
…mode #1933 Signed-off-by: Michael Ehab Mikhail <[email protected]>
@TG1999 I merged the recent fixes here and did another functional test for the v2 importer. |
…importer-package-first
Signed-off-by: Michael Ehab Mikhail <[email protected]>
def __init__(self, purl=None, *args, **kwargs): | ||
super().__init__(*args, **kwargs) | ||
self.purl = purl | ||
if self.purl: |
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 pattern is being used at multiple importers, we shall extract it out as a function
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 agree
We can modify the constructor of the base class instead of modifying each individual importer's constructor.
But I believe in this case we won't show the warning messages if the purl is not right for the importer.
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.
We can always pass the message or the type or even the log as a parameter
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 like the idea of passing the supported types, that would make it generic with less duplicate code
The warning message is the same, so it will adapt if we pass supported types
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.
@michaelehab there is no need to override __init__
in pipeline.
is_batch_run = True | ||
|
||
def __init__(self, *args, purl=None, **kwargs): | ||
super().__init__(*args, **kwargs) | ||
self.purl = purl | ||
if self.purl: | ||
ElixirSecurityImporterPipeline.is_batch_run = False | ||
if self.purl.type != "hex": | ||
self.log( | ||
f"Warning: PURL type {self.purl.type} is not 'hex', may not match any advisories" | ||
) | ||
|
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.
Do not add purl
as a primary argument to Pipeline. Primary arguments to the pipeline are exclusively reserved for managing pipeline execution. Instead, you can pass purl
to the pipeline like this: ElixirSecurityImporterPipeline(purl="pkg:hex/coherence")
and then access it inside a pipeline step using self.inputs["purl"]
.
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 see that the constructor has arguments for managing pipeline execution (in BasePipelineRun class).
How do I access the purl argument without overriding the constructor or modifying BasePipelineRun constructor?
The way I approached this was to call the parent constructor with the arguments so that the pipeline functions properly, and then take the PURL which is important in the importer-level.
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.
And based on @TG1999's comment, we can unify the purl handling in package-first mode by overriding the VulnerableCodeBaseImporterPipeline constructor for example to do something like this.
def __init__(self, *args, **kwargs):
super().__init__(*args, **kwargs)
self.purl = kwargs.get("purl")
self.supported_types = kwargs.get("supported_types)
Then doing the package-first checks and warnings which is unified for all importers.
Solves #1933