[WIP] Add manifest subcommand by sjug · Pull Request #5266 · containers/podman

sjug · 2020-02-19T16:06:31Z

Working towards #713.
Requires #5253.

Took a swing at breaking out the manifests monolith from buildah.
Quite a bit different implementation of the commands compared to podman GetRuntime() etc.

How much did you want to bring over from buildah vs refactor?
Do we want to lift getStore() from buildah to make it work as is?

I would appreciate some suggestions.

openshift-ci-robot · 2020-02-19T16:06:48Z

Hi @sjug. Thanks for your PR.

I'm waiting for a containers member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

openshift-ci-robot · 2020-02-19T16:10:01Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: sjug
To complete the pull request process, please assign mheon
You can assign the PR to them by writing /assign @mheon in a comment when ready.

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

Details

Needs approval from an approver in each of these files:

OWNERS

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

mheon · 2020-02-19T16:11:36Z

We can't use the Buildah store methods directly as they won't respect libpod.conf; will probably need to move some of this into pkg/image

rhatdan · 2020-02-21T12:30:28Z

@sjug First you need to sign your PRs to make the tests run.
It is fine to share as code with Buildah, we already do this for podman build and option parsing.
The question is, could we do the same for manifest options, so we don't end up with the options in two places.
Is there anything we could do with Buildah to make it easier to vendor manifests into podman.

Signed-off-by: Sebastian Jug <seb@stianj.ug>

sjug · 2020-02-24T16:14:32Z

I've kept the second commit to show how I replaced some key different functions from buildah to see if that's okay. Also the third commit is adding in the buildah/manifest deps.

I'm not sure why I'm failing gating?

baude · 2020-03-03T16:00:44Z

+	store := runtime.GetStore()
+	systemContext := runtime.SystemContext()
+
+	_, listImage, err := util.FindImage(store, "", systemContext, listImageSpec)


we typically would put this content and below into the adapter layer. I think given your not familar with that and we are actively working on it, I'm OK with ignoring that for now and allowing us to clean this up.

baude · 2020-03-03T16:02:05Z

+	store := runtime.GetStore()
+	systemContext := runtime.SystemContext()
+
+	_, listImage, err := util.FindImage(store, "", systemContext, listImageSpec)


we typically use something like runtime.LookupImage (or something close to that.)

baude · 2020-03-03T16:03:01Z

@@ -0,0 +1,166 @@
+package main


this command will need to fenced off as a local-only command until we have apiv2 endpoints for this. maybe that should get done now? so that this is a slightly cleaner implementation?

I prefer to defer the endpoints for manifest. We wanted to prevent libpod from importing buildah packages and I believe that's what we had to do.

baude · 2020-03-03T16:03:40Z

needs man pages and bash completion additions to pass gating.

vrothberg

First round of review looks good. We need tests for that, ideally the same as Buildah.

@nalind PTAL

vrothberg · 2020-03-05T11:14:19Z

+		return errors.New("At least a list image and an image to add must be specified")
+	case 2:
+		listImageSpec = args[0]
+		if listImageSpec == "" {


I prefer leaving these checks to the libraries.

vrothberg · 2020-03-05T11:59:47Z

@@ -0,0 +1,166 @@
+package main


I prefer to defer the endpoints for manifest. We wanted to prevent libpod from importing buildah packages and I believe that's what we had to do.

rhatdan · 2020-03-05T13:30:10Z

@vrothberg @sjug We are working on APIV2 for this first and setting up support for it in libpod. Once this happens we can work on the CLI for it. @baude Opened the first PR for APIV2 yesterday.

rh-atomic-bot · 2020-03-17T21:37:27Z

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

openshift-ci-robot · 2020-03-18T12:59:43Z

@sjug: PR needs rebase.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

github-actions · 2020-04-18T00:03:51Z

A friendly reminder that this PR had no activity for 30 days.

rhatdan · 2020-04-18T10:04:42Z

@sjug This PR is replaced with #5843

openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 19, 2020

sjug requested a review from rhatdan February 19, 2020 16:06

openshift-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Feb 19, 2020

openshift-ci-robot requested review from mrunalp and umohnani8 February 19, 2020 16:10

sjug force-pushed the add_manifest branch from 2481b3d to aba16a8 Compare February 19, 2020 16:13

sjug force-pushed the add_manifest branch 2 times, most recently from b536e40 to 3f39a0c Compare February 24, 2020 15:16

openshift-ci-robot added size/XL and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Feb 24, 2020

sjug force-pushed the add_manifest branch from 3f39a0c to 5b928f0 Compare February 24, 2020 15:52

openshift-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL labels Feb 24, 2020

Add manifests podman style

27f171f

Signed-off-by: Sebastian Jug <seb@stianj.ug>

sjug force-pushed the add_manifest branch from 5b928f0 to 1e102b5 Compare February 24, 2020 15:58

sjug added 2 commits February 24, 2020 11:04

Change runtime, clean manifest push & create

7b7c1cf

Signed-off-by: Sebastian Jug <seb@stianj.ug>

Add buildah manifests to vendor

2d83c56

Signed-off-by: Sebastian Jug <seb@stianj.ug>

sjug force-pushed the add_manifest branch from 1e102b5 to 2d83c56 Compare February 24, 2020 16:04

baude reviewed Mar 3, 2020

View reviewed changes

vrothberg reviewed Mar 5, 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 Mar 18, 2020

github-actions Bot added the stale-pr label Apr 18, 2020

rhatdan closed this Apr 18, 2020

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 25, 2023

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

Conversation

sjug commented Feb 19, 2020

Uh oh!

openshift-ci-robot commented Feb 19, 2020

Uh oh!

openshift-ci-robot commented Feb 19, 2020

Uh oh!

mheon commented Feb 19, 2020

Uh oh!

rhatdan commented Feb 21, 2020

Uh oh!

sjug commented Feb 24, 2020

Uh oh!

baude Mar 3, 2020

Choose a reason for hiding this comment

Uh oh!

baude Mar 3, 2020

Choose a reason for hiding this comment

Uh oh!

baude Mar 3, 2020

Choose a reason for hiding this comment

Uh oh!

vrothberg Mar 5, 2020

Choose a reason for hiding this comment

Uh oh!

baude commented Mar 3, 2020

Uh oh!

vrothberg left a comment

Choose a reason for hiding this comment

Uh oh!

vrothberg Mar 5, 2020

Choose a reason for hiding this comment

Uh oh!

vrothberg Mar 5, 2020

Choose a reason for hiding this comment

Uh oh!

rhatdan commented Mar 5, 2020

Uh oh!

rh-atomic-bot commented Mar 17, 2020

Uh oh!

openshift-ci-robot commented Mar 18, 2020

Uh oh!

github-actions Bot commented Apr 18, 2020

Uh oh!

rhatdan commented Apr 18, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants