spec: Rename app name to image name. #397

yifan-gu · 2015-05-07T23:39:41Z

Use app name in this section seems a little bit out of tune to me. How about changing it to image name?

/cc @jonboulle @philips

jonboulle · 2015-05-07T23:53:07Z

I think by and large this makes sense. One critical omission here: the "apps" field in the "dependencies" list.

yifan-gu · 2015-05-08T00:15:28Z

Updated.

jonboulle · 2015-05-08T00:35:48Z

SPEC.md

@@ -447,10 +447,10 @@ When evaluating `ac-discovery` tags, the client MUST first ensure that the prefi

 ```bash
 urltmpl="https://{name}-{version}-{os}-{arch}.{ext}"
-curl $(echo "$urltmpl" | sed -e "s/{name}/$appname/" -e "s/{version}/$version/" -e "s/{os}/$os/" -e "s/{arch}/$arch/" -e "s/{ext}/$ext/")
+curl $(echo "$urltmpl" | sed -e "s/{name}/$imgname/" -e "s/{version}/$version/" -e "s/{os}/$os/" -e "s/{arch}/$arch/" -e "s/{ext}/$ext/")


This should just be name, I think, not sure why it was appname in the first place?

jonboulle · 2015-05-08T00:44:32Z

@yifan-gu sorry, while you're in there..

yifan-gu · 2015-05-08T02:08:35Z

@jonboulle NW. Updated again.

jonboulle · 2015-05-08T02:10:51Z

SPEC.md

+These attributes are expressed in the **labels** field of the [Image Manifest](#image-manifest-schema).
+App Container Image Discovery prescribes a discovery process to retrieve an image based on the name and these attributes.
+
+


superfluous newline

jonboulle · 2015-05-08T02:11:56Z

@yifan-gu Spec change lgtm. Please update the schema alongside this, sorry I forgot to mention that earlier...
https://github.com/appc/spec/blob/master/schema/types/dependencies.go

yifan-gu · 2015-05-08T06:20:16Z

@jonboulle Then I found we need to change a bunch of places as well.... I have changed all the appearances of ImageID. PTAL.

jonboulle · 2015-05-09T01:19:03Z

/cc @krobertson any thoughts on this?

krobertson · 2015-05-09T16:48:05Z

LGTM, make more sense than "app name".

cdaylward · 2015-05-11T20:37:32Z

SPEC.md

@@ -762,8 +764,8 @@ JSON Schema for the Image Manifest (app image manifest, ACI manifest), conformin
        * **count** (integer, optional, defaults to 1) specifies a range of ports, starting with "port" and ending with "port" + "count" - 1.
        * **socketActivated** (boolean, optional, defaults to "false" if unsupplied) if set to true, the application expects to be [socket activated](http://www.freedesktop.org/software/systemd/man/sd_listen_fds.html) on these ports. The ACE must pass file descriptors using the [socket activation protocol](http://www.freedesktop.org/software/systemd/man/sd_listen_fds.html) that are listening on these ports when starting this app. If multiple apps in the same pod are using socket activation then the ACE must match the sockets to the correct apps using getsockopt() and getsockname().
 * **dependencies** (list of objects, optional) dependent application images that need to be placed down into the rootfs before the files from this image (if any). The ordering is significant. See [Dependency Matching](#dependency-matching) for how dependencies are retrieved.
-    * **app** (string, required) name of the dependent App Container Image.
-    * **imageID** (string of type [Image ID](#image-id-type), optional) content hash of the dependency. If provided, the retrieved dependency must match the hash. This can be used to produce deterministic, repeatable builds of an App Image that has dependencies.
+    * **name** (string of type [AC Name](#ac-name-type), required) name of the dependent App Container Image.


Would prefer to see this as "image". "name" seems to always be confusing because it is often ambiguous as to whether it is a reference to something else, or the name of the object itself (in this case, the name of the dependency).

@cdaylward my initial argument in favour of name would be that it is a key reference. But on thinking about it further it seems like:
a) this field should be optional; one might desire a deterministic set of dependencies (i.e. only referencing by ID) and the name field is not then strictly necessary (provided of course the implementation supports lookups based on ID alone)
b) a qualified imageName / imageID might be preferable since these are not actually image objects (rather, dependency objects)

@yifan-gu thoughts?

@jonboulle
For a), if the user also specifies the labels, then we can identify the image deterministically, no? My previous understanding on this is that sha512 is more for machine than for human.

For b), I agree that they are not image objects by definition, but they are still describing images. If that's pretty clear to user, or if images are the dependency object, then I think removing the 'image' part is OK. Furthermore, maybe we can call it "required images" instead of dependencies?

@yifan-gu There is nothing to guarantee that labels are deterministic, no. They should be, but we can't mandate it without having a centralised repository.

lets see, the content addressed iD presently implies a tar archive, which is an ACI archive.
"Dependencies" could be clarified a bit as "Required Images". This is a common pattern picked up in other packaging formats like RPM ("Requires").

Is there any other type of dependency besides images?

@vbatts

Is there any other type of dependency besides images?

Nope.

I'm sympathetic to "required images".

philips · 2015-05-15T02:30:28Z

lgtm, overall. I will let @jonboulle resolve the image/name discussion with @cdaylward and @yifan-gu

yifan-gu · 2015-05-15T19:39:55Z

@jonboulle I am ok with imageName, imageID, (do we need to change to imageLabels as well)?

vbatts · 2015-05-18T18:40:20Z

SPEC.md

-            "app": "example.com/reduce-worker-base",
-            "imageID": "sha512-...",
+            "name": "example.com/reduce-worker-base",
+            "id": "sha512-...",


perhaps this ID need a more generic name. It is defined as Image ID, but could likely be CAS ID or similar

@vbatts I don't really follow, how does this relate to the discussion below?

dropping this to ID here seems too terse. UUID is an ID as well, but that is not what is meant here. Sorry if it's off-topic, it just seems a little too terse. Like the next step would be to loosen up the requirement to be Image ID formatted.

Let's revert this to "imageID". Right now it does mean something rather strict. If we want to loosen the requirement we can address that separately.

sorry, I already started down this path but forgot to put up a PR - jonboulle@e343fa9

@jonboulle That looks good. You can send as a PR so I can close this :)

NV, Let me just pull from you commit

yifan-gu · 2015-05-20T22:06:26Z

Keeps the imageID, and change app to imageName.

jonboulle · 2015-05-21T00:24:21Z

@yifan-gu I don't think you just pulled my commit - the build is broken now :/

Also rename dependency.App to dependency.imageName.

yifan-gu · 2015-05-21T00:43:59Z

@jonboulle I did, and squashed the commits. For some reason we forgot this line :(
https://github.com/jonboulle/spec/blob/jon-imgname/pkg/acirenderer/acirenderer_test.go#L866

Now it's fixed

jonboulle · 2015-05-21T01:23:47Z

LGTM. Thanks.

spec: Rename app name to image name.

yifan-gu force-pushed the discovery branch from da4decb to 4a71e85 Compare May 7, 2015 23:45

yifan-gu force-pushed the discovery branch from 4a71e85 to c02028b Compare May 8, 2015 00:13

jonboulle reviewed May 8, 2015
View reviewed changes

yifan-gu force-pushed the discovery branch 3 times, most recently from 870aa4f to f8b6eea Compare May 8, 2015 02:07

jonboulle reviewed May 8, 2015
View reviewed changes

yifan-gu force-pushed the discovery branch from f8b6eea to 8b849e0 Compare May 8, 2015 06:17

cdaylward reviewed May 11, 2015
View reviewed changes

vbatts reviewed May 18, 2015
View reviewed changes

jonboulle mentioned this pull request May 18, 2015

*: v0.5.2 #411

Merged

yifan-gu force-pushed the discovery branch 2 times, most recently from e343fa9 to 3496fda Compare May 20, 2015 21:47

spec: Rename app name to image name.

ced5018

Also rename dependency.App to dependency.imageName.

yifan-gu force-pushed the discovery branch from 3496fda to ced5018 Compare May 21, 2015 00:42

jonboulle added a commit that referenced this pull request May 21, 2015

Merge pull request #397 from yifan-gu/discovery

52efa17

spec: Rename app name to image name.

jonboulle merged commit 52efa17 into appc:master May 21, 2015

yifan-gu deleted the discovery branch May 21, 2015 17:05

iaguis mentioned this pull request May 22, 2015

go get failed (Dependency field 'App' in struct literal) appc/docker2aci#62

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

spec: Rename app name to image name. #397

spec: Rename app name to image name. #397

yifan-gu commented May 7, 2015

jonboulle commented May 7, 2015

yifan-gu commented May 8, 2015

jonboulle May 8, 2015

jonboulle commented May 8, 2015

yifan-gu commented May 8, 2015

jonboulle May 8, 2015

jonboulle commented May 8, 2015

yifan-gu commented May 8, 2015

jonboulle commented May 9, 2015

krobertson commented May 9, 2015

cdaylward May 11, 2015

jonboulle May 11, 2015

yifan-gu May 12, 2015

jonboulle May 12, 2015

vbatts May 18, 2015

jonboulle May 19, 2015

philips commented May 15, 2015

yifan-gu commented May 15, 2015

vbatts May 18, 2015

jonboulle May 18, 2015

vbatts May 18, 2015

jonboulle May 19, 2015

yifan-gu May 20, 2015

jonboulle May 20, 2015

yifan-gu May 20, 2015

yifan-gu May 20, 2015

yifan-gu commented May 20, 2015

jonboulle commented May 21, 2015

yifan-gu commented May 21, 2015

jonboulle commented May 21, 2015

		These attributes are expressed in the labels field of the [Image Manifest](#image-manifest-schema).
		App Container Image Discovery prescribes a discovery process to retrieve an image based on the name and these attributes.

spec: Rename app name to image name. #397

spec: Rename app name to image name. #397

Conversation

yifan-gu commented May 7, 2015

jonboulle commented May 7, 2015

yifan-gu commented May 8, 2015

Choose a reason for hiding this comment

jonboulle commented May 8, 2015

yifan-gu commented May 8, 2015

Choose a reason for hiding this comment

jonboulle commented May 8, 2015

yifan-gu commented May 8, 2015

jonboulle commented May 9, 2015

krobertson commented May 9, 2015

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

philips commented May 15, 2015

yifan-gu commented May 15, 2015

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

yifan-gu commented May 20, 2015

jonboulle commented May 21, 2015

yifan-gu commented May 21, 2015

jonboulle commented May 21, 2015