-
Notifications
You must be signed in to change notification settings - Fork 37
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
Skip copying content for modules that have no execute #613
Conversation
@goldmann I'm going to add some info to the issue for discussion on this. |
I'm taking a look at the tests as well. |
@@ -609,7 +609,7 @@ def test_run_path_artifact_brew(tmpdir, caplog): | |||
|
|||
img_desc = image_descriptor.copy() | |||
img_desc['artifacts'] = [{'name': 'aaa', | |||
'md5': 'd41d8cd98f00b204e9800998ecf8427e', |
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.
Note this is a legit md5 in brew, so the fetch actually works, I changed it to something that doesn't exist (for now), but it might be better to pick something like: deadbeefdeadbeefdeadbeefdeadbeefdea not that that wouldn't occur either I suppose, but the intent is a bit clearer.
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 perfectly fine with it, thanks!
@goldmann I updated the tests to reflect the change, but I assume you may have a strong reaction to this change ;). I definitely think that its a bug to add the copy in the case of no content, however. |
FYI: I created a tracker issue: #614. |
@@ -609,7 +609,7 @@ def test_run_path_artifact_brew(tmpdir, caplog): | |||
|
|||
img_desc = image_descriptor.copy() | |||
img_desc['artifacts'] = [{'name': 'aaa', | |||
'md5': 'd41d8cd98f00b204e9800998ecf8427e', |
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 perfectly fine with it, thanks!
@@ -733,8 +733,6 @@ def test_execution_order(tmpdir): | |||
|
|||
###### START module 'child_3:1.0' | |||
###### \\ | |||
# Copy 'child_3' module content | |||
COPY modules/child_3 /tmp/scripts/child_3 |
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 guess this is the only pat that I'm not sure how to tackle, but I guess it will be rare to see a module without any envs/labels and scripts to execute.
@luck3y Thank you, merged! |
See #593
Fixes #614.