app/add: Use the image name as a default name for app #3802
Conversation
rkt/app_add.go
Outdated
| } | ||
| partsSlash := strings.Split(partsColon[0], "/") | ||
| if len(partsSlash) < 1 { | ||
| stderr.Println("invalid image name, cannot use it as an app name") |
There was a problem hiding this comment.
This seems to be missing a return 254?
|
IMO instead of parsing the url by hand you should use ParseDockerURL from +1 for extracting that to separate func with proper name and a docstring. Also - missing unittest ;) |
|
@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. |
|
@lucab I haven't found any, but I need to take a deeper look at |
d9b12e9 to
30f6c81
Compare
common/common.go
Outdated
| @@ -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 | |||
rkt/app_add.go
Outdated
| @@ -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])) | |||
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
30f6c81 to
0cf0c05
Compare
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
0cf0c05 to
91fc588
Compare
|
@iaguis done |
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