manifest create,add,inspect by QiWang19 · Pull Request #5843 · containers/podman

QiWang19 · 2020-04-16T14:04:46Z

Implememts manifest subcommands create, add, inspect.

jwhonce · 2020-04-16T17:23:36Z

Suggested change

case 0, 1:

return errors.New("At least a list image and an image to add must be specified")

Args: cobra.MinimumNArgs(2) Ensures this case will never be true.

jwhonce · 2020-04-16T17:25:37Z

Given this default, I would change to Args.ExactArgs(2) and remove the switch.

jwhonce · 2020-04-16T17:27:08Z

Suggested change

if len(args) == 0 {

return errors.New("At least a name must be specified for the list")

}

``

Args: cobra.MinimumNArgs(1) ensures this is always false.

jwhonce · 2020-04-16T17:29:10Z

Suggested change

switch len(args) {

case 0:

return errors.New("At least a source list ID must be specified")

case 1:

if args[0] == "" {

return errors.Errorf(`Invalid image name "%s"`, args[0])

}

default:

return errors.New("Only one argument is necessary for inspect: an image name")

}

Args: cobra.ExactArgs(1) will take care of this switch

jwhonce · 2020-04-16T17:30:23Z

Suggested change

PersistentPreRunE: preRunE,

As of #5848 this is no longer required.

jwhonce · 2020-04-16T17:33:58Z

Cannot use Printf() in this layer. Need to return data to caller.

jwhonce · 2020-04-16T17:34:50Z

Suggested change

_, err = alltransports.ParseImageName(fmt.Sprintf("%s%s", dockerPrefix, name))

_, err = alltransports.ParseImageName(dockerPrefix + name)

jwhonce · 2020-04-16T17:35:45Z

Suggested change

fmt.Printf("%s\n", buf)

return buf, nil

This needs to be returned to caller, not printed at this layer.

jwhonce · 2020-04-16T17:37:34Z

Suggested change

ManifestCreate(ctx context.Context, names, images []string, opts ManifestCreateOptions) error

ManifestInspect(ctx context.Context, name string) error

ManifestAdd(ctx context.Context, opts ManifestAddOptions) error

ManifestCreate(ctx context.Context, names, images []string, opts ManifestCreateOptions) (<report>, error)

ManifestInspect(ctx context.Context, name string) (<report>, error)

ManifestAdd(ctx context.Context, opts ManifestAddOptions) (<report>, error)

You cannot present data to user in these functions, needs to be done in the manifest/*.go files.

jwhonce · 2020-04-16T17:39:17Z

You should print out err to help with debugging issues.

I checked other tests. Integration tests don't log errors in BeforeEach(). Does this need for manifest?

rh-atomic-bot · 2020-04-16T18:57:55Z

☔ The latest upstream changes (presumably #5848) made this pull request unmergeable. Please resolve the merge conflicts.

QiWang19 · 2020-04-20T20:01:40Z

@jwhonce PTAL

QiWang19 · 2020-04-22T20:20:30Z

@jwhonce @rhatdan @mheon @TomSweeneyRedHat PTAL

TomSweeneyRedHat · 2020-04-22T23:23:00Z

Did you mean to have an extra couple of spaces before the last three lines here?

TomSweeneyRedHat · 2020-04-22T23:23:23Z

TomSweeneyRedHat · 2020-04-22T23:23:42Z

TomSweeneyRedHat · 2020-04-22T23:31:34Z

A couple of spacing/tab nits, otherwise LGTM

Implememts manifest subcommands create, add, inspect. Signed-off-by: Qi Wang <qiwan@redhat.com>

rhatdan · 2020-04-23T08:29:25Z

/approve

openshift-ci-robot · 2020-04-23T08:29:29Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: QiWang19, rhatdan

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [rhatdan]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

rhatdan · 2020-04-23T08:32:35Z

+	flags := addCmd.Flags()
+	flags.BoolVar(&manifestAddOpts.All, "all", false, "add all of the list's images if the image is a list")
+	flags.StringSliceVar(&manifestAddOpts.Annotation, "annotation", nil, "set an `annotation` for the specified image")
+	flags.StringVar(&manifestAddOpts.Arch, "arch", "", "override the `architecture` of the specified image")


Buildah has --os as well?

Need to change libpod/image/manifests.go to support --os, I can fix it in another PR.

rhatdan · 2020-04-23T08:38:06Z

Buildah also has
buildah-manifest-annotate.1 buildah-manifest-push.1
buildah-manifest-remove.1

QiWang19 · 2020-04-23T15:25:25Z

Buildah also has
buildah-manifest-annotate.1 buildah-manifest-push.1
buildah-manifest-remove.1

I plan to finish the rest implementation in a different PR.

rhatdan · 2020-04-23T15:42:33Z

/lgtm

openshift-ci-robot requested review from mheon and umohnani8 April 16, 2020 14:04

jwhonce suggested changes Apr 16, 2020

View reviewed changes

openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 17, 2020

QiWang19 force-pushed the manifest_create branch from 3a20523 to 42ba884 Compare April 17, 2020 18:10

openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 17, 2020

QiWang19 changed the title ~~podmanV2:manifest create,add,inspect~~ manifest create,add,inspect Apr 17, 2020

QiWang19 force-pushed the manifest_create branch 7 times, most recently from 5d1c59d to 768bd6b Compare April 17, 2020 20:59

rhatdan mentioned this pull request Apr 18, 2020

[WIP] Add manifest subcommand #5266

Closed

QiWang19 force-pushed the manifest_create branch from 768bd6b to f5e5801 Compare April 20, 2020 20:01

QiWang19 force-pushed the manifest_create branch 4 times, most recently from cc2ff7d to 8880f60 Compare April 22, 2020 19:46

TomSweeneyRedHat reviewed Apr 22, 2020

View reviewed changes

Comment thread completions/bash/podman Outdated

Copy link
Copy Markdown

Member

TomSweeneyRedHat Apr 22, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

spacing?

TomSweeneyRedHat reviewed Apr 22, 2020

View reviewed changes

Comment thread completions/bash/podman Outdated

Copy link
Copy Markdown

Member

TomSweeneyRedHat Apr 22, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

spacing?

manifest create,add,inspect

17783dd

Implememts manifest subcommands create, add, inspect. Signed-off-by: Qi Wang <qiwan@redhat.com>

QiWang19 force-pushed the manifest_create branch from 8880f60 to 17783dd Compare April 23, 2020 00:05

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 23, 2020

rhatdan reviewed Apr 23, 2020

View reviewed changes

openshift-ci-robot assigned rhatdan Apr 23, 2020

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 23, 2020

openshift-merge-robot merged commit 397dcc3 into containers:master Apr 23, 2020

QiWang19 deleted the manifest_create branch June 26, 2020 15:12

github-actions Bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Sep 24, 2023

github-actions Bot locked as resolved and limited conversation to collaborators Sep 24, 2023

	case 0, 1:
	return errors.New("At least a list image and an image to add must be specified")

-	if len(args) == 0 {
-		return errors.New("At least a name must be specified for the list")
-	}
+``
+Args: cobra.MinimumNArgs(1) ensures this is always false.

-	switch len(args) {
-	case 0:
-		return errors.New("At least a source list ID must be specified")
-	case 1:
-		if args[0] == "" {
-			return errors.Errorf(`Invalid image name "%s"`, args[0])
-		}
-	default:
-		return errors.New("Only one argument is necessary for inspect: an image name")
-	}

	_, err = alltransports.ParseImageName(fmt.Sprintf("%s%s", dockerPrefix, name))
	_, err = alltransports.ParseImageName(dockerPrefix + name)

-	ManifestCreate(ctx context.Context, names, images []string, opts ManifestCreateOptions) error
-	ManifestInspect(ctx context.Context, name string) error
-	ManifestAdd(ctx context.Context, opts ManifestAddOptions) error
+	ManifestCreate(ctx context.Context, names, images []string, opts ManifestCreateOptions) (<report>, error)
+	ManifestInspect(ctx context.Context, name string)  (<report>, error)
+	ManifestAdd(ctx context.Context, opts ManifestAddOptions)  (<report>, error)

Conversation

QiWang19 commented Apr 16, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rh-atomic-bot commented Apr 16, 2020

Uh oh!

QiWang19 commented Apr 20, 2020

Uh oh!

QiWang19 commented Apr 22, 2020

Uh oh!

TomSweeneyRedHat Apr 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TomSweeneyRedHat commented Apr 22, 2020

Uh oh!

rhatdan commented Apr 23, 2020

Uh oh!

openshift-ci-robot commented Apr 23, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rhatdan commented Apr 23, 2020

Uh oh!

QiWang19 commented Apr 23, 2020

Uh oh!

rhatdan commented Apr 23, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

TomSweeneyRedHat Apr 22, 2020 •

edited

Loading