Skip to content
This repository has been archived by the owner on Aug 14, 2020. It is now read-only.

discovery: fix HasPrefix in matching app name #594

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

alban
Copy link
Member

@alban alban commented Apr 8, 2016

The following meta:

  <meta name="ac-discovery" content="coreos.com/etcd https://github.com/coreos/etcd/releases/download/{version}/etcd-{version}-{os}-{arch}.{ext}">

Should make the following discoverable:

  actool discover coreos.com/etcd/fdfdsffds:v2.0.10
  actool discover coreos.com/etcd:v2.0.10

But not the following:

  actool discover coreos.com/etcdxxxxxxx:v2.0.10

This is now fixed by checking the character '/' in addition to
HasPrefix.

Fixes #592

The following meta:
  <meta name="ac-discovery" content="coreos.com/etcd https://github.com/coreos/etcd/releases/download/{version}/etcd-{version}-{os}-{arch}.{ext}">

Should make the following discoverable:
  actool discover coreos.com/etcd/fdfdsffds:v2.0.10
  actool discover coreos.com/etcd:v2.0.10

But not the following:
  actool discover coreos.com/etcdxxxxxxx:v2.0.10

This is now fixed by checking the character '/' in addition to
HasPrefix.

Fixes appc#592
@sgotti
Copy link
Member

sgotti commented Apr 8, 2016

Interesting case.

Now the spec just uses prefix-match and says to ensure that the prefix of the AC Name being discovered matches the prefix-match, so it needs probably to be changed since this patch implements a more strict behavior.

@philips
Copy link
Contributor

philips commented Apr 8, 2016

Hrm, I think this is unfortunate but expected. Once the client gets the image it would find the name doesn't match.

@jonboulle
Copy link
Contributor

I think we should just update #594 (comment) to clarify that it should be either be an exact or filepath.Dir() match, rather than HasPrefix() , no?

@sgotti
Copy link
Member

sgotti commented Apr 12, 2016

Actually it isn't really causing a problem (since in #592 rkt doesn't complain due to the --insecure-options=image).

But keeping prefix-match and changing the logic seems confusing (since it's a path/dir match)

@jonboulle
Copy link
Contributor

@sgotti are you suggesting just leaving this as-is?

@sgotti
Copy link
Member

sgotti commented Apr 12, 2016

@jonboulle I'm on the fence. On one side this isn't causing problems if the discovery url consumer checks the image manifest name with the provided name, on the other side doing a path matching instead of a prefix match makes more sense (in my previous sentence I tried to say that, if this is going to be changed, there's the need to find a better replacement for the prefix-match terms inside https://github.com/appc/spec/blob/master/spec/discovery.md)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

"actool discover" accepts non existing images
4 participants