manifest push using yaml file #866

clnperez · 2018-02-08T02:46:13Z

Instead of:
manifest create list image1 image2
manifest annotate list image1
manifest push list

You can do:
manifest push --file=myfile.yaml list:tag

Signed-off-by: Christy Norman christy@linux.vnet.ibm.com

- What I did

- How I did it

- How to verify it

- Description for the changelog

- A picture of a cute animal (not mandatory but encouraged)

clnperez · 2018-02-08T02:46:39Z

I still need to add a test. If anyone wants to give this a quick once-over in the meantime feel free.

clnperez · 2018-02-08T02:49:56Z

Also, so I don't forget, I need to update the doc to add this, and and to include how to set the cli experimental. It's confusing, because it's "true" for the engine, but "enabled" for the cli. :D

dnephin · 2018-02-08T22:38:00Z

cli/command/manifest/push.go

 	return cmd
 }

 func runPush(dockerCli command.Cli, opts pushOpts) error {

+	if opts.target == "" && opts.file == "" {
+		return errors.New("please specify either a file or local manifest name")


I believe we generally avoid "please" in error messages. Maybe "either a filename or local manifest name is required" or something like that?

dnephin · 2018-02-08T22:39:37Z

cli/command/manifest/push.go

+		}
+		if err := annotateListFromYaml(dockerCli, yamlInput); err != nil {
+			return err
+		}


It seems a bit odd to me that push would create local files. could this push directly from yaml without creating the files locally?

If we want to support "load defaults from yaml" or something like that we could have create accept the same yaml format (either in this PR or a separate one).

dnephin · 2018-02-08T22:42:40Z

cli/command/manifest/push.go

-	flags.BoolVar(&opts.insecure, "insecure", false, "Allow push to an insecure registry")
+	flags.BoolVarP(&opts.purge, "purge", "p", false, "remove the local manifest list after push")
+	flags.BoolVar(&opts.insecure, "insecure", false, "allow push to an insecure registry")
+	flags.StringVar(&opts.file, "filename", "", "create, annotate, and push using a yaml file")


I wonder if file should be a boolean flag that changes the positional argument from a "local manifest target" to "a filepath". I'm not sure if that's more or less expected than having a conditional argument.

I think it would at least make the validation easier, because with a bool flag a positional argument would always be necessary.

dnephin · 2018-02-08T22:45:37Z

cli/command/manifest/push.go

+
+	if _, err := os.Stat(yamlFile); err != nil {
+		return yamlManifestList{}, err
+	}


Is this Stat necessary? I think you'd get the same error from ReadFile().

nope, thanks!

dnephin · 2018-02-08T22:46:36Z

cli/command/manifest/push.go

+	if err != nil {
+		return yamlManifestList{}, err
+	}
+	if err := yaml.UnmarshalStrict(yamlBuf, &yamlInput); err != nil {


This if is unnecessary:

return yamlInput, yaml.UnmarshalStrict(yamlBuf, &yamlInput)

clnperez · 2018-06-01T02:52:00Z

Redid this (no longer uses local fs) but haven't tested the original path yet, so I'll get to that on Monday, and also take the rest of @dnephin's comments into account. Sorry for abandoning this for so long!

clnperez · 2018-06-06T22:26:54Z

tests added for yaml. removing [WIP]

@vdemeester @thaJeztah the week before dockercon, so, you've got time to take a look at this now right? 👼

clnperez · 2018-06-06T22:31:05Z

cc @tianon @StefanScherer @AceHack

vdemeester

I really like the idea, but I feel docker push --file=true my.yaml is not correct.

The current behavior is docker manifest push an-image-ref-which-is-a-list, so… for the file, I really feels we should have something more like docker manifest push --file=my.yaml an-image-ref-which-is-a-list.

cc @thaJeztah @silvin-lubecki @dnephin

StefanScherer · 2018-06-07T09:00:53Z

Thanks @clnperez I'll look into it in the next days. Haven't looked into the details, but agree that pushing a yaml should be done with the docker manifest push command.

clnperez · 2018-06-07T14:16:22Z

@vdemeester i wasn't a huge fan of the file change but it grew on me and did make validation easier. i'm open to changes for sure. but i'm not sure i like having someone specify any information on the command line that could be also in a yaml file (in this case, the target ref).

vdemeester · 2018-06-07T15:06:15Z

@clnperez I really like the file, but in the current proposal there is two things that I feel could be better

cli UX, a flag should not change the meaning of the positional args ; e.g. in the current design, having --file=true makes the positionnal arg being either a list (from the manifest store) or a file.
the list is a image reference, and I think even with the file, we should specify that image reference as positional arguments. It would make the file not tied to the image reference pushed — it would be possible to do something like : docker manifest push --file=foo.yml vdemeester/foo && docker manifest push --file=foo.yml grc.io/vdemeester/foo — and the command would not be far from the current usage (we just add the possibility to pass a file)

So my proposal is to make the file something like the following :

manifests:
    - image: test/hello-world-ppc64le:latest
      platform:
          architecture: ppc64le
    - image: test/hello-world-amd64:latest
      platform:
          architecture: amd64
          os: linux
    - image: test/hello-world-s390x:latest
      platform:
          architecture: s390x 
          os: linux
          osversion: 1.1
          variant: xyz
          osfeatures: [a,b,c]

And have the cli being docker manifest push --file=myfile.yaml list.

@thaJeztah wdyt ?

dnephin · 2018-06-07T16:06:49Z

I agree with @vdemeester

clnperez · 2018-06-07T18:51:13Z

@vdemeester can you clarify: the file not tied to the image reference pushed...? The image reference pushed is the first line in the file the way the manifest tool works, and the way I intended this one to work as well. What's the meaning of vdemeester/foo in your example? Would it overwrite the target image on the first line of the file?

vdemeester · 2018-06-07T20:38:14Z

@clnperez yes, my sugestion would be to not have the image: … first line in the file (so just listing in the manifest file the manifests).

I updated my example above as I realized I did not remove that first line 😅.

clnperez · 2018-06-07T21:18:39Z

Thanks @vdemeester. That's a simple enough code change. I'd like to hear from some others before we get attached. :D

silvin-lubecki

Some nits

silvin-lubecki · 2018-06-06T12:54:34Z

cli/command/manifest/push.go

+	if opts.file {
+		return pushListFromYaml(dockerCli, opts.target, opts.insecure)
+	}
+


nit: too much empty lines

silvin-lubecki · 2018-06-06T12:55:28Z

cli/command/manifest/push.go

+		return err
+	}
+	if len(yamlInput.Manifests) == 0 {
+		return errors.Errorf("no manifests specified in file input")


nit: errors.Errorf is not needed here. You can use errors.New instead.

silvin-lubecki · 2018-06-06T12:57:17Z

cli/command/manifest/push.go

+}
+
+func addYamlAnnotations(manifest *types.ImageManifest, ym yamlManifest) {
+


nit: empty line

silvin-lubecki · 2018-06-06T12:57:29Z

cli/command/manifest/push.go

@@ -271,3 +292,69 @@ func mountBlobs(ctx context.Context, client registryclient.RegistryClient, ref r
 	}
 	return nil
 }
+
+func pushListFromYaml(dockerCli command.Cli, file string, insecure bool) error {
+


nit: empty line

silvin-lubecki · 2018-06-07T08:58:43Z

cli/command/manifest/push.go

+	if opts.file {
+		return pushListFromYaml(dockerCli, opts.target, opts.insecure)
+	}
+


nit: empty line

silvin-lubecki · 2018-06-08T12:55:08Z

cli/command/manifest/push.go

+}
+
+func getYamlManifestList(yamlFile string) (yamlManifestList, error) {
+	var yamlInput yamlManifestList


nit: you can move the var declaration closer to where it is used.

tianon · 2018-06-13T19:00:00Z

I think it's relevant to bring up estesp/manifest-tool#23 (multiple tags pushed at once; estesp/manifest-tool#32) in the context of removing image: from the YAML itself -- being able to simply docker manifest push --file xyz.yml abc:1.2.3 abc:1.2 abc:1 abc:latest would be great (especially if it's efficient about skipping the expensive digest lookups and cross-blob mounts).

It probably won't be very useful for pushing to multiple registries though (hub+gcr.io for example), right? (since the manifest list image references still need to be on the same registry for cross-repository blob-mounting)

estesp/manifest-tool#22 is also relevant for the official images use-case -- we have to wait to push the manifest lists until the full set of architectures have built (see docker-library/official-images#3835), so having --ignore-missing helps us to be able to push a manifest list in the meantime, but it's only a workaround for a much larger problem in the way manifest list objects have to be created and updated (namely, the race conditions around "updating" a manifest list without API support for adding/updating entries directly to an existing tag's manifest list in an idempotent way at the registry level, which would allow us to build/update manifest lists more fluidly as architectures build and push like they were before manifest lists were implemented and we were single-arch).

It'd be neat to be able to specify this YAML content without needing a file on-disk, but that's more of a minor annoyance than a serious issue (since it's a matter of garbage collection after we build the appropriate YAML, push the bits, then clean up afterwards).

clnperez · 2018-06-20T18:27:01Z

Thanks @tianon . I think for this PR, I can look at just redoing the --file arg so that it points to the list name (instead of it living in the yaml file), and have two follow-on PRs:

add support for extra optional args that specify other refs to name this same manifest list, so it's all done with only one need to cross-mount the layer blobs.
add the --ignore-missing parameter

clnperez · 2018-07-02T21:02:01Z

@silvin-lubecki nits taken into account, thanks :)

@vdemeester i'll have to redo this again if we go with #1156. Unless we want to merge this one first and make @dmcgowan rebase his. ;)

for now though -- i can't get the vendor bit fixed
make -f docker.Makefile vendor ?

clnperez · 2018-07-02T21:49:46Z

figured out vendor. i think i had switched a commit. PTAL

clnperez · 2018-07-03T19:30:18Z

Rebased against master post-merge of #1156

clnperez · 2018-07-10T13:52:06Z

ping @thaJeztah @vdemeester

codecov-io · 2018-08-02T14:26:21Z

Codecov Report

Merging #866 into master will increase coverage by 0.07%.
The diff coverage is 82.14%.

@@            Coverage Diff            @@
##           master    #866      +/-   ##
=========================================
+ Coverage   54.63%   54.7%   +0.07%     
=========================================
  Files         293     293              
  Lines       19369   19418      +49     
=========================================
+ Hits        10582   10623      +41     
- Misses       8126    8130       +4     
- Partials      661     665       +4

clnperez · 2018-08-02T14:58:19Z

Rebased. PTAL.

clnperez · 2018-08-29T18:31:23Z

paging @thaJeztah & @vdemeester

vdemeester

Design LGTM 👼

cli/command/manifest/push.go

silvin-lubecki

LGTM, thank you @clnperez !

silvin-lubecki · 2018-09-06T15:08:21Z

@thaJeztah are you ok to merge this PR?

Instead of: manifest create list image1 image2 manifest annotate list image1 manifest push list You can do: manifiest push --file myfile.yaml registry/repo/image:tag Signed-off-by: Christy Norman <christy@linux.vnet.ibm.com>

clnperez · 2018-09-19T19:33:54Z

ping @thaJeztah . would love to close this 😇

duglin · 2018-11-05T13:37:00Z

@dnephin @vdemeester can chance of merging this one?

clnperez · 2018-11-29T16:38:57Z

ping @thaJeztah. I saw that #1252 was merged so I have a sliver of hope that someone will take a look at this again.

olljanat · 2018-11-29T16:49:12Z

Nice is very good idea. Would be nice to get this merged before #1355

grooverdan · 2019-02-26T21:36:48Z

Nice work @clnperez, great way to create manifests by yaml definition.

vdemeester

Still LGTM for me
cc @thaJeztah @andrewhsu

GordonTheTurtle added the status/0-triage label Feb 8, 2018

clnperez mentioned this pull request Feb 8, 2018

Add manifest command #138

Merged

1 task

dnephin reviewed Feb 8, 2018

View reviewed changes

vdemeester added status/1-design-review and removed status/0-triage labels Feb 9, 2018

clnperez force-pushed the manifest-list-yaml branch 2 times, most recently from 988d7e0 to 4c2bba3 Compare June 1, 2018 02:51

clnperez force-pushed the manifest-list-yaml branch 4 times, most recently from c37ebc3 to b25349f Compare June 6, 2018 22:21

clnperez changed the title ~~[WIP] manifest push using yaml file~~ manifest push using yaml file Jun 6, 2018

vdemeester requested changes Jun 7, 2018

View reviewed changes

silvin-lubecki reviewed Jun 8, 2018

View reviewed changes

clnperez force-pushed the manifest-list-yaml branch from b25349f to 7bc7d2a Compare June 20, 2018 18:22

clnperez force-pushed the manifest-list-yaml branch from 7bc7d2a to ae7bcd6 Compare July 2, 2018 18:55

clnperez requested a review from thaJeztah as a code owner July 2, 2018 18:55

clnperez force-pushed the manifest-list-yaml branch from ae7bcd6 to 317c10b Compare July 2, 2018 19:00

clnperez force-pushed the manifest-list-yaml branch 3 times, most recently from 368e4c9 to c9a9950 Compare July 2, 2018 21:44

clnperez force-pushed the manifest-list-yaml branch from c9a9950 to 12b3a0f Compare July 3, 2018 19:07

clnperez force-pushed the manifest-list-yaml branch from 12b3a0f to 06cc41f Compare August 2, 2018 14:20

vdemeester approved these changes Sep 6, 2018

View reviewed changes

cli/command/manifest/push.go Outdated Show resolved Hide resolved

silvin-lubecki approved these changes Sep 6, 2018

View reviewed changes

manifest push using yaml file

dbd1fce

Instead of: manifest create list image1 image2 manifest annotate list image1 manifest push list You can do: manifiest push --file myfile.yaml registry/repo/image:tag Signed-off-by: Christy Norman <christy@linux.vnet.ibm.com>

clnperez force-pushed the manifest-list-yaml branch from 06cc41f to dbd1fce Compare September 6, 2018 15:09

olljanat mentioned this pull request Dec 17, 2018

docker manifest commands don't work on 18.03.0-ce-rc3 #947

Closed

vdemeester approved these changes Feb 27, 2019

View reviewed changes

		}

		func addYamlAnnotations(manifest *types.ImageManifest, ym yamlManifest) {

manifest push using yaml file #866

Are you sure you want to change the base?

manifest push using yaml file #866

Conversation

clnperez commented Feb 8, 2018 • edited Loading

clnperez commented Feb 8, 2018

clnperez commented Feb 8, 2018

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

clnperez commented Jun 1, 2018 • edited Loading

clnperez commented Jun 6, 2018

clnperez commented Jun 6, 2018

vdemeester left a comment

Choose a reason for hiding this comment

StefanScherer commented Jun 7, 2018

clnperez commented Jun 7, 2018

vdemeester commented Jun 7, 2018 • edited Loading

dnephin commented Jun 7, 2018

clnperez commented Jun 7, 2018

vdemeester commented Jun 7, 2018

clnperez commented Jun 7, 2018

silvin-lubecki left a 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

tianon commented Jun 13, 2018

clnperez commented Jun 20, 2018

clnperez commented Jul 2, 2018

clnperez commented Jul 2, 2018

clnperez commented Jul 3, 2018 • edited Loading

clnperez commented Jul 10, 2018

codecov-io commented Aug 2, 2018 • edited Loading

Codecov Report

clnperez commented Aug 2, 2018

clnperez commented Aug 29, 2018

vdemeester left a comment

Choose a reason for hiding this comment

silvin-lubecki left a comment

Choose a reason for hiding this comment

silvin-lubecki commented Sep 6, 2018

clnperez commented Sep 19, 2018 • edited Loading

duglin commented Nov 5, 2018

clnperez commented Nov 29, 2018

olljanat commented Nov 29, 2018

grooverdan commented Feb 26, 2019

vdemeester left a comment

Choose a reason for hiding this comment

clnperez commented Feb 8, 2018 •

edited

Loading

clnperez commented Jun 1, 2018 •

edited

Loading

vdemeester commented Jun 7, 2018 •

edited

Loading

clnperez commented Jul 3, 2018 •

edited

Loading

codecov-io commented Aug 2, 2018 •

edited

Loading

clnperez commented Sep 19, 2018 •

edited

Loading