added support to parse labels in dockerfile #3987
Conversation
VarshaUN
force-pushed
the
support-OCI-labels
branch
from
edd1a8d to
c90069a
Compare
AyanSinhaMahapatra
left a comment
AyanSinhaMahapatra
left a comment
There was a problem hiding this comment.
@VarshaUN Thanks++
Looking good, need another round of changes and a lot more tests here for the added functionality, and this should be much better.
VarshaUN
force-pushed
the
support-OCI-labels
branch
from
d60d0c8 to
328639b
Compare
VarshaUN
force-pushed
the
support-OCI-labels
branch
from
e82bc6c to
7a785ea
Compare
|
@AyanSinhaMahapatra I have added everything that you told me. Please review it. |
VarshaUN
force-pushed
the
support-OCI-labels
branch
5 times, most recently
from
a7ba60e to
adf690a
Compare
Signed-off-by: Ayan Sinha Mahapatra <[email protected]> Signed-off-by: Jono Yang <[email protected]> Signed-off-by: Jono Yang <[email protected]> addded support to parse labels in dockerfile Signed-off-by: Varsha U N <[email protected]>
VarshaUN
force-pushed
the
support-OCI-labels
branch
from
adf690a to
610689c
Compare
AyanSinhaMahapatra
left a comment
AyanSinhaMahapatra
left a comment
There was a problem hiding this comment.
Thanks++ @VarshaUN this is looking great! A few more comments for your consideration. Thank you for your patience
Can you also remove this file which was added with your PR: tests/packagedcode/__pycache__/test_parse_pyproject_toml.cpython-312-pytest-8.3.3.pyc.26520?
Please also make sure you look into test failures (many are failing as an effect of this PR) and try to resolve them if they are related to your PR, also merge from develop since it's been a while.
| from packagedcode.dockerfile import DockerfileHandler | ||
|
|
||
| class TestDockerfileHandler: | ||
|
|
There was a problem hiding this comment.
Can you also run one full scan for one case, like the test here: https://github.com/aboutcode-org/scancode-toolkit/blob/develop/tests/packagedcode/test_cargo.py#L158, one is enough, don't have to do this for all files
| expected_packages = self.load_expected(expected_loc) | ||
| assert packages == expected_packages | ||
|
|
||
| def test_extract_oci_labels_from_dockerfile(self, mocker): |
There was a problem hiding this comment.
There is an issue here, how is the files generated by this test different from the test functions test_parse_dockerfile above? You are using the same filenames for the test expectations so we are missing a set of test files.
We want to have two sets of tests distinctly both for DockerfileHandler.parse and DockerfileHandler.extract_oci_labels_from_dockerfile and this should generate 2 sets of files so 2 docker/container files and total 6 test expectation files. You might want to use -package.expected.json and -expected.json to differentiate there.
Signed-off-by: Varsha U N <[email protected]>
AyanSinhaMahapatra
left a comment
AyanSinhaMahapatra
left a comment
There was a problem hiding this comment.
@VarshaUN thanks for the updates, please fix all test failures. Looks good and ready to merge otherwise.
tests/packagedcode/data/docker/test-dockerfile/test.dockerfile-package.expected.json
Outdated
Show resolved
Hide resolved
Signed-off-by: Varsha U N <[email protected]>
Signed-off-by: Varsha U N <[email protected]>
3 participants
Fixes #3561
Tasks
Signed-off-by: Varsha U N [email protected]