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.

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

Only pull images that can't build `docker-compose pull` #6494

Merged
merged 1 commit into from Mar 25, 2019

Conversation

Projects
None yet
4 participants
@collin5
Copy link
Contributor

collin5 commented Feb 3, 2019

Signed-off-by: Collins Abitekaniza abtcolns@gmail.com

Resolves #6464

@collin5 collin5 force-pushed the collin5:b6464 branch from dfbd8d9 to e439c72 Feb 3, 2019

@GordonTheTurtle

This comment has been minimized.

Copy link

GordonTheTurtle commented Feb 3, 2019

Please sign your commits following these rules:
https://github.com/moby/moby/blob/master/CONTRIBUTING.md#sign-your-work
The easiest way to do this is to amend the last commit:

$ git clone -b "b6464" git@github.com:collin5/compose.git somewhere
$ cd somewhere
$ git rebase -i HEAD~842354283488
editor opens
change each 'pick' to 'edit'
save the file and quit
$ git commit --amend -s --no-edit
$ git rebase --continue # and repeat the amend for each commit
$ git push -f

Amending updates the existing PR. You DO NOT need to open a new one.

@collin5 collin5 force-pushed the collin5:b6464 branch from 891530f to 346ee75 Feb 3, 2019

@GordonTheTurtle GordonTheTurtle removed the dco/no label Feb 3, 2019

@collin5 collin5 force-pushed the collin5:b6464 branch from 346ee75 to e439c72 Feb 3, 2019

@collin5

This comment has been minimized.

Copy link
Contributor Author

collin5 commented Feb 3, 2019

Blocked by #6490

@collin5 collin5 changed the title Only pull images that can't build `docker-compose pull` [WIP] Only pull images that can't build `docker-compose pull` Feb 3, 2019

@collin5 collin5 changed the title [WIP] Only pull images that can't build `docker-compose pull` Only pull images that can't build `docker-compose pull` Feb 3, 2019

@collin5 collin5 force-pushed the collin5:b6464 branch from e439c72 to 9014122 Feb 6, 2019

@jose-pvargas
Copy link

jose-pvargas left a comment

Hey @collin5! This would be a great addition to docker-compose. I left some friendly feedback that I think would improve the readability of the code, especially to someone new to the project, such as myself 😄 . Please let me know what you think!

@@ -602,6 +602,12 @@ def _get_convergence_plans(self, services, strategy, always_recreate_deps=False)
def pull(self, service_names=None, ignore_pull_failures=False, parallel_pull=False, silent=False,
include_deps=False):
services = self.get_services(service_names, include_deps)
can_build_images = [service.image_name for service in filter(

This comment has been minimized.

Copy link
@jose-pvargas

jose-pvargas Mar 13, 2019

I find that the syntax of filter,map, and reduce are a bit counter intuitive in the sense that they reverse the order of the subject and the predicate. So instead of reading give me the images that can be built they read more like, if the image can be built, give me the image.

If I were scanning through the code, the current name can_build_images gives me an impression that this would be referencing a boolean value but it is actually referencing a list of image names. I think that we can give this variable a name closer to what it actually refers to.

Additionally, as this is being used to identify which services to pull, we should use a set not a list, for O(1) look-ups as opposed to O(n) with lists.

For that reason, I think that the following is more readable and more efficient.

images_to_build = {service.image_name for service in services if service.can_be_built()}

This comment has been minimized.

Copy link
@collin5

collin5 Mar 23, 2019

Author Contributor

Nice catch! Yeah, I agree with you, especially on the naming part 🤦‍♂️ . Thank you.

can_build_images = [service.image_name for service in filter(
lambda service: service.can_be_built(), services)]

can_pull = filter(lambda service: not service.can_be_built()

This comment has been minimized.

Copy link
@jose-pvargas

jose-pvargas Mar 13, 2019

If a service can be built, it will already be in can_build_images, so we can simplify the boolean condition.

I suggest the following (using images_to_build in place of can_build_images:

services_to_pull = [service for service in services if service.image_name not in images_to_build]
only pull images that can't build
Signed-off-by: Collins Abitekaniza <abtcolns@gmail.com>

@collin5 collin5 force-pushed the collin5:b6464 branch from 9014122 to c6dd7da Mar 23, 2019

@collin5

This comment has been minimized.

Copy link
Contributor Author

collin5 commented Mar 23, 2019

@jose-pvargas I've updated this. Thanks again.

@ijc

ijc approved these changes Mar 25, 2019

Copy link
Contributor

ijc left a comment

LGTM thanks.

@ijc ijc merged commit 6ccbb56 into docker:master Mar 25, 2019

2 checks passed

continuous-integration/jenkins/pr-head This commit looks good
Details
dco-signed All commits are signed

@collin5 collin5 deleted the collin5:b6464 branch Mar 25, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.