app/add: Use the image name as a default name for app #3802

nhlfr · 2017-09-19T08:23:37Z

The help message in "app add" sucommand suggests that --name flag is optional, but it wasn't. This commit fixes the desired behaviour.

Fixes #3801

lucab

Good catch. However I'd probably move the splitting logic in its own func defaultAppName(imageName string) (string, error) helper.

lucab · 2017-09-19T08:35:26Z

rkt/app_add.go

+		}
+		partsSlash := strings.Split(partsColon[0], "/")
+		if len(partsSlash) < 1 {
+			stderr.Println("invalid image name, cannot use it as an app name")


This seems to be missing a return 254?

jellonek · 2017-09-19T10:03:31Z

IMO instead of parsing the url by hand you should use ParseDockerURL from appc/docker2aci which is already used e.x. in pkg/distribution/docker.go.

+1 for extracting that to separate func with proper name and a docstring.

Also - missing unittest ;)

lucab · 2017-09-19T11:00:08Z

@jellonek I guess the input is an ACI name though. I'm not sure if we already have some helper around in appc to extract the last component.

nhlfr · 2017-09-19T11:50:40Z

@lucab I haven't found any, but I need to take a deeper look at rkt run. Maybe we can split the login into a helper and use it both in rkt add and rkt app add.

nhlfr · 2017-09-28T13:07:21Z

@lucab ok, we've found the function that rkt run uses to extract a default app name from image name. That helper function was in stage0/ package, I moved it to common/

@jellonek tests added ;)

@iaguis PTAL

iaguis

Some comments.

iaguis · 2017-09-28T13:10:28Z

common/common.go

@@ -539,3 +539,17 @@ func GetOSArch() (os string, arch string) {
 	os, arch, _ = types.ToAppcOSArch(runtime.GOOS, arch, flavor)
 	return os, arch
 }
+
+// ImageNameToAppName converts the full nale of image to an app name without special


nale -> name

iaguis · 2017-09-28T13:31:16Z

rkt/app_add.go

@@ -111,6 +112,15 @@ func runAppAdd(cmd *cobra.Command, args []string) (exit int) {
 	}
 	rktApps.Last().ImageID = *img

+	if rktApps.Last().Name == "" {
+		appName, err := common.ImageNameToAppName(types.ACIdentifier(args[1]))


Not sure I like that app names include the version or result on weird things like etcd-v2-0-0-linux-amd64-aci since it's not consistent with anything we do.

You can add

app.Name = appName.String()

here https://github.com/rkt/rkt/pull/3802/files#diff-71a7d6d46a80cd343d272145a7de0cf2L68

And get rid of the changes in this file (rkt/app_add.go) and things should work. So https://github.com/coreos/etcd/releases/download/v2.0.0/etcd-v2.0.0-linux-amd64.aci will have a name of etcd and coreos.com/etcd:v2.0.10 too.

Wdyt?

I don't understand it. Why we need to do anything more in stage0? What change brings the explicit conversion to string? Will it magically cut versions from the name? I don't think so, if it doesn't work like that in our unit test - and the change you proposed is not going to change results of that unit test.

My point is we're already calling ImageNameToAppName in stage0, but we're not using the name afterwards. And I don't want names like these:

75545b2a etcd-v2-0-10 coreos.com/etcd:v2.0.10 exited 14 minutes ago 14 minutes ago etcd-v2-0-0-linux-amd64-aci coreos.com/etcd:v2.0.0

Maybe it's easier to explain with code: kinvolk@13f0944

To complete my comment, ImageNameToAppName should be applied to image names, not to image "URLs" like coreos.com/etcd:v2.0.10 or https://github.com/coreos/etcd/releases/download/v2.0.0/etcd-v2.0.0-linux-amd64.aci and that's what we're doing in rkt/app_add.go

The help message in "app add" sucommand suggests that --name flag is optional, but it wasn't. This commit fixes the desired behaviour. Fixes rkt#3801

nhlfr · 2017-09-28T15:09:28Z

@iaguis done

iaguis

Thanks. LGTM if green

lucab suggested changes Sep 19, 2017

View reviewed changes

lucab added area/cri component/stage0 needs/some-changes labels Sep 19, 2017

lucab added this to the 1.29.0 milestone Sep 19, 2017

nhlfr force-pushed the nhlfr/app-name-default branch 4 times, most recently from d9b12e9 to 30f6c81 Compare September 28, 2017 13:04

iaguis suggested changes Sep 28, 2017

View reviewed changes

nhlfr force-pushed the nhlfr/app-name-default branch from 30f6c81 to 0cf0c05 Compare September 28, 2017 15:05

app/add: Use the image name as a default name for app

91fc588

The help message in "app add" sucommand suggests that --name flag is optional, but it wasn't. This commit fixes the desired behaviour. Fixes rkt#3801

nhlfr force-pushed the nhlfr/app-name-default branch from 0cf0c05 to 91fc588 Compare September 28, 2017 15:06

iaguis approved these changes Sep 28, 2017

View reviewed changes

iaguis added reviewed/merge-on-green and removed needs/some-changes labels Sep 28, 2017

iaguis merged commit 0134aff into rkt:master Sep 28, 2017

nhlfr deleted the nhlfr/app-name-default branch September 28, 2017 16:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

app/add: Use the image name as a default name for app #3802

app/add: Use the image name as a default name for app #3802

app/add: Use the image name as a default name for app #3802

app/add: Use the image name as a default name for app #3802

Conversation

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