Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add support for Windows containers #181

Merged
merged 30 commits into from
Aug 4, 2021
Merged

Add support for Windows containers #181

merged 30 commits into from
Aug 4, 2021

Conversation

pombredanne
Copy link
Contributor

No description provided.

tdruez
tdruez reviewed May 27, 2021
View reviewed changes
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
Copy link
Contributor

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?

Copy link
Contributor

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!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tdruez there are some issues that @JonoYang uncovered in how code is being extracted AFAIRC.

Copy link
Contributor

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, #251 added

tdruez
tdruez reviewed May 27, 2021
View reviewed changes
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
Copy link
Contributor

@tdruez tdruez May 27, 2021 •
edited
Loading

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.

Copy link
Contributor Author

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.

tdruez
tdruez requested changes Jul 16, 2021
View reviewed changes
etc/requirements/base.txt Outdated Show resolved Hide resolved
scanpipe/pipelines/docker.py Outdated Show resolved Hide resolved
scanpipe/pipelines/windows_docker.py Outdated Show resolved Hide resolved
@tdruez
Copy link
Contributor

tdruez commented Jul 16, 2021

@JonoYang we need docstrings for all pipeline steps and pipes as parts of the documentation are automatically generated from that content.

@JonoYang
Copy link
Contributor

@tdruez Thanks for the review. I added/updated docstrings to the functions that were missing it.

pombredanne
pombredanne commented Jul 23, 2021
View reviewed changes
scanpipe/pipes/docker.py Outdated Show resolved Hide resolved
@pombredanne pombredanne mentioned this pull request Jul 25, 2021
Closed
@JonoYang JonoYang force-pushed the windows-containers branch 3 times, most recently from 275cdd6 to 439a7fe Compare July 28, 2021 00:53
@JonoYang JonoYang force-pushed the windows-containers branch 4 times, most recently from b6ca6a6 to 2fa7470 Compare July 30, 2021 22:52
@pombredanne pombredanne mentioned this pull request Aug 2, 2021
Closed
pombredanne and others added 10 commits August 2, 2021 10:29
@pombredanne @JonoYang
Use newer version of container libraries
354bf8b 
Signed-off-by: Philippe Ombredanne <pombredanne@nexb.com>
@pombredanne @JonoYang
Use new container-inspector structures
624f8cd 
Signed-off-by: Philippe Ombredanne <pombredanne@nexb.com>
@pombredanne @JonoYang
Add minimal support for Windows containers
927bd4b 
Signed-off-by: Philippe Ombredanne <pombredanne@nexb.com>
@JonoYang
Update Windows package getter
ee5ea1b 
    * The windows_helper module from scancode is not available on pypi

Signed-off-by: Jono Yang <jyang@nexb.com>
@pombredanne @JonoYang
Use newer version of container libraries
fcfbe3c 
Signed-off-by: Philippe Ombredanne <pombredanne@nexb.com>
@JonoYang
Update call to windows_helper to win_reg
7c66011 
Signed-off-by: Jono Yang <jyang@nexb.com>
@JonoYang
Create new pipeline for Windows Docker images
324cf3d 
    * Create Windows specific tag_uninteresting_windows_codebase_resources function

Signed-off-by: Jono Yang <jyang@nexb.com>
@JonoYang
Add function to find packages at well-known paths
4f1eac7 
    * Update tests

Signed-off-by: Jono Yang <jyang@nexb.com>
@JonoYang
Add step to tag known software in pipeline
198efed 
    * Change name of Docker step from "find_images_linux_distro" to "find_images_os_and_distro"

Signed-off-by: Jono Yang <jyang@nexb.com>
@JonoYang
Get version from path in tag_known_software #238
3a49932 
    * Update docstrings
    * Pin fetchcode dep

Signed-off-by: Jono Yang <jyang@nexb.com>
JonoYang and others added 13 commits August 2, 2021 10:31
@JonoYang
Troubleshoot regex patterns #238
1b46251 
Signed-off-by: Jono Yang <jyang@nexb.com>
@JonoYang
Report Program File contents as packages #238
c80f06b 
Signed-off-by: Jono Yang <jyang@nexb.com>
@JonoYang
Update Windows-specific regex
358b9ac 
    * Add more file names and file extensions to be ignored
    * Update expected test results

Signed-off-by: Jono Yang <jyang@nexb.com>
@JonoYang
Do not ignore .mui files #238
a632464 
Signed-off-by: Jono Yang <jyang@nexb.com>
@JonoYang
Filter using extension field rather than path #238
431d1e4 
Signed-off-by: Jono Yang <jyang@nexb.com>
@pombredanne @JonoYang
Update scanpipe/pipes/docker.py
04d45aa 
Create issue to track extraction issue

See #251

Signed-off-by: Philippe Ombredanne <pombredanne@nexb.com>
@JonoYang
Fix scancode-toolkit pinned version in base.txt #238
9c74c4c 
Signed-off-by: Jono Yang <jyang@nexb.com>
@JonoYang
Create pipeline step to tag ignorable files #252
4636576 
Signed-off-by: Jono Yang <jyang@nexb.com>
@JonoYang
Update formatting #238
b216c6e 
Signed-off-by: Jono Yang <jyang@nexb.com>
@JonoYang
Generalize regex expressions #238
aaafc04 
    * 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>
@JonoYang
Create new pipes for ignoring files #238
730e808 
    * Create pipes that ignore media files and data files with no clues
    * Update test results

Signed-off-by: Jono Yang <jyang@nexb.com>
@JonoYang
Add more file extensions to ignore #238
546296f 
Signed-off-by: Jono Yang <jyang@nexb.com>
@JonoYang
Bump dep versions #238
6baaeb0 
Signed-off-by: Jono Yang <jyang@nexb.com>
@JonoYang
Update docstring #238
76c9e4f 
    * Use InstalledWindowsProgram object instead of Package

Signed-off-by: Jono Yang <jyang@nexb.com>
@JonoYang
Improve regex used in tag_known_software #238
2220809 
    * Update tests with more paths to test regex patterns

Signed-off-by: Jono Yang <jyang@nexb.com>
tdruez added a commit that referenced this pull request Aug 3, 2021
@tdruez
Adjust code for consistency across the codebase #181
ff0f3fd
@tdruez
Adjust code for consistency across the codebase #181
a48eb4c 
Signed-off-by: Thomas Druez <tdruez@nexb.com>
tdruez
tdruez requested changes Aug 3, 2021
View reviewed changes
Copy link
Contributor

@tdruez tdruez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@JonoYang good work, looks like we're almost there for the merge.
I've made a round of adjustments in a48eb4c

See my comments for a last round of code improvements.

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 = (
Copy link
Contributor

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.

Copy link
Contributor

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, that sounds good.

Copy link
Contributor

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):
Copy link
Contributor

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?

Copy link
Contributor

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=[]):
Copy link
Contributor

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(
Copy link
Contributor

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

Copy link
Contributor

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.

Copy link
Contributor

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()

@JonoYang
Address PR comments #238
c94fc15 
    * 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>
@JonoYang
Add is_media field to CodebaseResource #238
ced8f38 
    * Update test results

Signed-off-by: Jono Yang <jyang@nexb.com>
JonoYang and others added 2 commits August 3, 2021 12:38
@JonoYang
Simplify tag_media_files_as_unintersting() #238
53b128a 
    * Update test

Signed-off-by: Jono Yang <jyang@nexb.com>
@tdruez
Refine windows pipes #238
2645864 
Signed-off-by: Thomas Druez <tdruez@nexb.com>
@tdruez tdruez merged commit a7e3897 into main Aug 4, 2021
@tdruez tdruez deleted the windows-containers branch August 4, 2021 07:10
@tdruez
Copy link
Contributor

tdruez commented Aug 4, 2021

@JonoYang nice work, PR merged!
See 2645864#diff-30934e49c9b2107144b0ea45fd86cb0693bf7544f0195a3caec127e0118720a1R96 for a better fix for empty list as default parameter.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tdruez tdruez tdruez requested changes

@JonoYang JonoYang JonoYang left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@pombredanne @tdruez @JonoYang