-
Notifications
You must be signed in to change notification settings - Fork 85
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
Add support for Windows containers #181
Conversation
scanpipe/pipes/docker.py
Outdated
@@ -61,23 +61,23 @@ def extract_layers_from_images(project, images): | |||
Return the `errors` that may have happen during the extraction. | |||
""" | |||
errors = [] | |||
|
|||
# FIXME: use container-inspector extract instead |
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.
Could you provide some context and reason about this change?
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.
@JonoYang Could you enter a new issue with some details of the problem? This would have more chance to be worked on than a comment in the code.
Thanks!
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.
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.
@pombredanne I would prefer a ticket nonetheless as those # FIXME
tend to never get dealt with.
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.
Good point, #251 added
etc/requirements/base.txt
Outdated
@@ -24,14 +24,14 @@ redis==3.5.3 | |||
gunicorn==20.1.0 | |||
|
|||
# Docker | |||
container_inspector>=3.1.2 | |||
container_inspector>=21.5.25 |
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.
Using >=
with CalVer is problematic for API changes, see 7b7fc2b
One that would run the install at the moment would encounter issues running the docker pipeline:
'Image' object has no attribute 'base_location'
We should revert back to exact versions.
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's a container-inspector issue ... and that's my making: this ship has sailed there now and I cannot see how I can revert there.
Note that the API for this container-inspector library is fast evolving but I will thrive to keep it backward compatible as much as possible.
28be790
to
91074cf
Compare
8ac5529
to
720091c
Compare
@JonoYang we need docstrings for all pipeline steps and pipes as parts of the documentation are automatically generated from that content. |
@tdruez Thanks for the review. I added/updated docstrings to the functions that were missing it. |
275cdd6
to
439a7fe
Compare
b6ca6a6
to
2fa7470
Compare
Signed-off-by: Philippe Ombredanne <pombredanne@nexb.com>
Signed-off-by: Philippe Ombredanne <pombredanne@nexb.com>
Signed-off-by: Philippe Ombredanne <pombredanne@nexb.com>
* The windows_helper module from scancode is not available on pypi Signed-off-by: Jono Yang <jyang@nexb.com>
Signed-off-by: Philippe Ombredanne <pombredanne@nexb.com>
Signed-off-by: Jono Yang <jyang@nexb.com>
* Create Windows specific tag_uninteresting_windows_codebase_resources function Signed-off-by: Jono Yang <jyang@nexb.com>
* Update tests Signed-off-by: Jono Yang <jyang@nexb.com>
* Change name of Docker step from "find_images_linux_distro" to "find_images_os_and_distro" Signed-off-by: Jono Yang <jyang@nexb.com>
* Update docstrings * Pin fetchcode dep Signed-off-by: Jono Yang <jyang@nexb.com>
Signed-off-by: Jono Yang <jyang@nexb.com>
Signed-off-by: Jono Yang <jyang@nexb.com>
* Add more file names and file extensions to be ignored * Update expected test results Signed-off-by: Jono Yang <jyang@nexb.com>
Signed-off-by: Jono Yang <jyang@nexb.com>
Signed-off-by: Jono Yang <jyang@nexb.com>
Create issue to track extraction issue See #251 Signed-off-by: Philippe Ombredanne <pombredanne@nexb.com>
Signed-off-by: Jono Yang <jyang@nexb.com>
Signed-off-by: Jono Yang <jyang@nexb.com>
Signed-off-by: Jono Yang <jyang@nexb.com>
* Modify regex used for Windows container analysis so it can be used outside the context of a Windows Docker image * Update tests Signed-off-by: Jono Yang <jyang@nexb.com>
* Create pipes that ignore media files and data files with no clues * Update test results Signed-off-by: Jono Yang <jyang@nexb.com>
Signed-off-by: Jono Yang <jyang@nexb.com>
Signed-off-by: Jono Yang <jyang@nexb.com>
2fa7470
to
6baaeb0
Compare
* Use InstalledWindowsProgram object instead of Package Signed-off-by: Jono Yang <jyang@nexb.com>
cc1cfc4
to
e65b6da
Compare
* Update tests with more paths to test regex patterns Signed-off-by: Jono Yang <jyang@nexb.com>
e65b6da
to
2220809
Compare
Signed-off-by: Thomas Druez <tdruez@nexb.com>
ff0f3fd
to
a48eb4c
Compare
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.
scanpipe/pipes/rootfs.py
Outdated
`mimes` and `types` are taken from TypeCode: | ||
https://github.com/nexB/typecode/blob/main/src/typecode/contenttype.py#L528 | ||
""" | ||
mimes = ( |
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 be directly imported from typecode
.
It would require to make https://github.com/nexB/typecode/blob/main/src/typecode/contenttype.py#L528 available as a module variable.
In the short term, we can keep it as-is and enter a ticket on the typecode
side.
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.
@tdruez On the other hand, what if we added the is_media
field from the license scan to the CodebaseResource
model?
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.
Sure, that sounds good.
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've added the is_media
field to CodebaseResource
, removed the part that excludes is_media
from unsupported_fields
in scanpipe.pipes.scancode.get_resource_info
, and updated the tests results.
scanpipe/pipelines/windows_docker.py
Outdated
from scanpipe.pipes import windows | ||
|
||
|
||
class WindowsDocker(Docker): |
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.
What about DockerWindows
and docker_windows
instead? This would keep all Docker based pipeline grouped in the UI.
I'm not sure about this though, what's your take?
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 think it makes sense to rename the pipeline to DockerWindows
for grouping.
scanpipe/pipes/windows.py
Outdated
qs.filter(lookups).update(status="ignored-not-interesting") | ||
|
||
|
||
def tag_installed_package_files(project, root_dir_pattern, package, q_objects=[]): |
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 use mutable default arguments.
scanpipe/pipes/windows.py
Outdated
|
||
openjdk_versions_by_path = {} | ||
for openjdk_codebase_resource in qs.filter(rootfs_path__regex=openjdk_root_pattern): | ||
_, openjdk_root_path, _, _, _, openjdk_version, _, _, _ = re.split( |
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.
What about using the match system https://docs.python.org/3/library/re.html#match-objects for improved readability and future maintenance?
Some examples in https://github.com/package-url/packageurl-python/blob/master/src/packageurl/contrib/url2purl.py
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 applies to the 3 re.split()
of the PR.
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 replaced all useage of re.split()
with re.match()
* Use re.match instead of re.split * Rename WindowsDocker pipeline to DockerWindows * Set the default value of the q_objects argument for tag_installed_package_files to be a tuple Signed-off-by: Jono Yang <jyang@nexb.com>
* Update test results Signed-off-by: Jono Yang <jyang@nexb.com>
eac2426
to
ced8f38
Compare
* Update test Signed-off-by: Jono Yang <jyang@nexb.com>
Signed-off-by: Thomas Druez <tdruez@nexb.com>
@JonoYang nice work, PR merged! |
No description provided.