Fix push command skips service #7430

musikov · 2020-05-05T16:46:44Z

Resolves #7429

The first service is skipped because it has no build instruction - it's ok.
But his image still was added to unique_images set, so the second service (with same image, but with build instruction) was skipped.
I've fixed this by adding check is service is pushable. Only after this i add it to unique_images

…en't pushed Signed-off-by: Mykyta Usikov <mykyta.usikov@gmail.com>

Signed-off-by: Mykyta Usikov <mykyta.usikov@gmail.com>

compose/project.py

thaJeztah · 2020-05-06T07:01:27Z

compose/service.py

+        return 'image' in self.options
+
+    def can_be_pushed(self):
+        return 'build' in self.options and 'image' in self.options


Not sure if there's a validation for this already, but image can also have a digest in it (e.g. busybox@sha256:a8cf7ff6367c2afa2a90acd081b484cbded349a7076e7bdf37a05279f276bc12)

While that's valid for pulling an image, it's not valid for building or pushing an image; I think this function should check if parse_repository_tag returns digest_separator (@) as separator, and if so, return false.

Probably something like this.

def can_be_pushed(self): if 'build' not in self.options or 'image' not in self.options: return False _, _, separator = parse_repository_tag(self.options.get('image')) return separator != '@'

Can you also add a test for these?

On a side-note: from my limited Python skills it looks like currently, parse_repository_tag does not support a reference to have both a tag and a digest, for example:

busybox:latest@sha256:a8cf7ff6367c2afa2a90acd081b484cbded349a7076e7bdf37a05279f276bc12

admittedly, that syntax is mostly is to make the reference "nicer" to read, because digest is leading in that case, but it probably should be looked at

compose/compose/service.py

Lines 1443 to 1445 in 440c94e

if digest_separator in repo_path:

repo, tag = repo_path.rsplit(digest_separator, 1)

return repo, tag, digest_separator

/cc @ulyssessouza

Not sure if there's a validation for this already, but image can also have a digest in it (e.g. busybox@sha256:a8cf7ff6367c2afa2a90acd081b484cbded349a7076e7bdf37a05279f276bc12)

While that's valid for pulling an image, it's not valid for building or pushing an image; I think this function should check if parse_repository_tag returns digest_separator (@) as separator, and if so, return false.

Probably something like this.

def can_be_pushed(self): if 'build' not in self.options or 'image' not in self.options: return False _, _, separator = parse_repository_tag(self.options.get('image')) return separator != '@'

Can you also add a test for these?

Well, if someone stated, that service has build phase and image with digest, it's a mistake in config. push handler shouldn't be bothered by it, the client will fail on push with explanation.
The better way is to add such checks in config validation IMHO

(update) suggestion to leave can_be_pushed same, but add exception if trying to push image with digest

def can_be_pushed(self): return 'build' in self.options and 'image' in self.options ... def push(self, ignore_push_failures=False): if not self.can_be_pushed(): return repo, tag, separator = parse_repository_tag(self.options['image']) if separator == '@': raise ConfigurationError('Can\'t push image with digest') tag = tag or 'latest' log.info('Pushing %s (%s%s%s)...' % (self.name, repo, separator, tag)) output = self.client.push(repo, tag=tag, stream=True) ...

On a side-note: from my limited Python skills it looks like currently, parse_repository_tag does not support a reference to have both a tag and a digest, for example:

busybox:latest@sha256:a8cf7ff6367c2afa2a90acd081b484cbded349a7076e7bdf37a05279f276bc12

admittedly, that syntax is mostly is to make the reference "nicer" to read, because digest is leading in that case, but it probably should be looked at

compose/compose/service.py

Lines 1443 to 1445 in 440c94e

if digest_separator in repo_path:

repo, tag = repo_path.rsplit(digest_separator, 1)

return repo, tag, digest_separator

/cc @ulyssessouza

What is expected behaviour? Jsut ignore tag, so

>>> parse_repository_tag('user/repo:v1@sha256:digest') ('user/repo', 'sha256:digest', '@')

I think pulling with just the digest is ok (digest references can only be used for pulling / running, not for pushing/building). Unless we want this function to return all things separate (which may be more "correct", but would require more changes);

This is "ok" image@sha256:deadbeef

But more "correct" would be

repo, tag, digest = parse_repository_tag('user/repo:v1@sha256:digest')

(return the tag (if any) and/or digest (if any)); perhaps it should return some struct having those properties.

But (as mentioned) for now, just using image@sha256:deadbeef should be ok

But (as mentioned) for now, just using image@sha256:deadbeef should be ok

Added #7432 pull request for it, 'cause it's separate functionality

thaJeztah · 2020-05-06T07:25:47Z

compose/project.py

@@ -695,7 +695,9 @@ def push(self, service_names=None, ignore_push_failures=False):
            repo, tag, sep = parse_repository_tag(service.image_name)
            service_image_name = sep.join((repo, tag)) if tag else sep.join((repo, 'latest'))


Perhaps service.image_name should be modified to do this (note: I'm not a Python developer, so perhaps there's reasons why that wasn't done 😅);

compose/compose/service.py

Lines 386 to 389 in 440c94e

def image_name(self):

return self.options.get('image', '{project}_{s.name}'.format(

s=self, project=self.project.lstrip('_-')

))

Which could look something like (again, I'm not a python developer, so possibly there's a cleaner way):

def image_name(self): image = self.options.get('image', '{project}_{s.name}'.format( s=self, project=self.project.lstrip('_-') )) if image == "": return repo, tag, separator = parse_repository_tag(image) tag = tag or 'latest' return repo+separator+tag

Why do we need to do so? Just to ensure tag is not empty?

There's some old behavior where docker push image<no :tag here> pushes all tags for image. In this case though, omitting :tag means :latest, so I think it would make sense to return that here (so that this logic doesn't have to be repeated everywhere); open to suggestions though

If we look through whole code, we can see, that latest is added only after parse_repository_tag call. So maybe we should add it there?
Anyways, think we need separate issue/discussion for it (despite it can simplify solving current issue)

compose/project.py

thaJeztah · 2020-05-06T07:40:02Z

tests/unit/project_test.py

+            'busy1', image=BUSYBOX_IMAGE_WITH_TAG,
+            client=self.mock_client)
+        svc_with_build = Service(
+            'busy2', image=BUSYBOX_IMAGE_WITH_TAG,


See my comment about digests; can you add a test-case that has a service with a digest in the image reference?

If we add config validation for "build + digest image", we won't need this check

thaJeztah · 2020-05-06T07:47:25Z

compose/service.py

@@ -1250,7 +1256,7 @@ def pull(self, ignore_pull_failures=False, silent=False, stream=False):
        return progress_stream.get_digest_from_pull(event_stream)

    def push(self, ignore_push_failures=False):
-        if 'image' not in self.options or 'build' not in self.options:
+        if not self.can_be_pushed():


Wondering; should we log something if the service does have an image: specified, but either doesn't have build: or image has a digest set (to give some clue why the image for a service wasn't pushed)?

It would be great to have logs!
For services without build and|or image just info log, that they were skipped.

thaJeztah · 2020-05-06T07:54:41Z

compose/project.py

-            if service_image_name not in unique_images:
+            # Add service to unique if it's pushable
+            if (service_image_name not in unique_images and
+                    service.can_be_pushed()):
                service.push(ignore_push_failures)


Unrelated: actually never given much attention to ignore_push_failures, but I think ignore_push_failures is way too broad; it seems to ignore any push failure (for whatever cause push failed). I think it'd make sense to either (by default?) ignore missing images (e.g., if I didn't build a service yet and the image isn't present), or add / rename this option to --ignore-missing.

I don't think other failures should be ignorable (e.g. authentication failed, registry unreachable, or pushing caused any other failure)

@ulyssessouza WDYT?

Opened #7431 to propose adding an --ignore-missing or --skip-missing flag

Signed-off-by: Mykyta Usikov <mykyta.usikov@gmail.com>

domibarton · 2020-11-25T16:59:15Z

Is there anybody working at this MR, because we're running into the same issue :)

musikov · 2020-12-03T15:01:55Z

Is there anybody working at this MR, because we're running into the same issue :)

I've changed service order in my compose file and forgot about this issue.

I'll soon return to it, review requested changes

glours · 2022-07-27T13:54:46Z

Thanks for taking the time to create this issue/pull request!

Unfortunately, Docker Compose V1 has reached end-of-life and we are not accepting any more changes (except for security issues). Please try and reproduce your issue with Compose V2 or rewrite your pull request to be based on the v2 branch and create a new issue or PR with the relevant Compose V2 information.

Fix push command skipped services with existing imagees, but that wer…

a64973f

…en't pushed Signed-off-by: Mykyta Usikov <mykyta.usikov@gmail.com>

musikov requested review from ndeloof, rumpl and ulyssessouza as code owners May 5, 2020 16:46

Fix import order in project_test.py

cc2172f

Signed-off-by: Mykyta Usikov <mykyta.usikov@gmail.com>

musikov force-pushed the 7429-push-skips-service branch from 6cbecb2 to cc2172f Compare May 5, 2020 17:10

joseangel-sc reviewed May 6, 2020

View reviewed changes

compose/project.py Outdated Show resolved Hide resolved

thaJeztah requested changes May 6, 2020

View reviewed changes

thaJeztah mentioned this pull request May 6, 2020

[RFC] deprecate push --ignore-push-failures and implement --skip-missing or --ignore-missing #7431

Open

musikov mentioned this pull request May 6, 2020

Ignore image tag if both tag and digest available #7432

Closed

Removed redundant comment and brackets

ec6ac6a

Signed-off-by: Mykyta Usikov <mykyta.usikov@gmail.com>

ndeloof added the Docker Compose V1 label Nov 24, 2021

glours closed this Jul 27, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix push command skips service #7430

Fix push command skips service #7430

musikov commented May 5, 2020

thaJeztah May 6, 2020

thaJeztah May 6, 2020

musikov May 6, 2020

musikov May 6, 2020 •

edited

musikov May 6, 2020 •

edited

thaJeztah May 6, 2020

musikov May 6, 2020

thaJeztah May 6, 2020

musikov May 6, 2020

thaJeztah May 6, 2020

musikov May 6, 2020

thaJeztah May 6, 2020

musikov May 6, 2020

thaJeztah May 6, 2020

musikov May 6, 2020

thaJeztah May 6, 2020

thaJeztah May 6, 2020

domibarton commented Nov 25, 2020

musikov commented Dec 3, 2020 •

edited

glours commented Jul 27, 2022

	if digest_separator in repo_path:
	repo, tag = repo_path.rsplit(digest_separator, 1)
	return repo, tag, digest_separator

		@@ -695,7 +695,9 @@ def push(self, service_names=None, ignore_push_failures=False):
		repo, tag, sep = parse_repository_tag(service.image_name)
		service_image_name = sep.join((repo, tag)) if tag else sep.join((repo, 'latest'))

	def image_name(self):
	return self.options.get('image', '{project}_{s.name}'.format(
	s=self, project=self.project.lstrip('_-')
	))

Fix push command skips service #7430

Fix push command skips service #7430

Conversation

musikov commented May 5, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

musikov May 6, 2020 • edited

Choose a reason for hiding this comment

musikov May 6, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

domibarton commented Nov 25, 2020

musikov commented Dec 3, 2020 • edited

glours commented Jul 27, 2022

musikov May 6, 2020 •

edited

musikov May 6, 2020 •

edited

musikov commented Dec 3, 2020 •

edited