Skip to content

podmanV2: implement build#5819

Merged
openshift-merge-robot merged 1 commit into
containers:masterfrom
vrothberg:v2-build
Apr 17, 2020
Merged

podmanV2: implement build#5819
openshift-merge-robot merged 1 commit into
containers:masterfrom
vrothberg:v2-build

Conversation

@vrothberg
Copy link
Copy Markdown
Member

@vrothberg vrothberg commented Apr 15, 2020

Implement podman build for the local client. The remote client will
require some rather large work in the backend and a new build endpoint
for the libpod rest API.

Signed-off-by: Valentin Rothberg rothberg@redhat.com

@openshift-ci-robot
Copy link
Copy Markdown
Collaborator

openshift-ci-robot commented Apr 15, 2020

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: vrothberg

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:

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

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 15, 2020
@vrothberg
Copy link
Copy Markdown
Member Author

vrothberg commented Apr 15, 2020

@baude @mheon @jwhonce

@rh-atomic-bot
Copy link
Copy Markdown
Collaborator

rh-atomic-bot commented Apr 15, 2020

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

@vrothberg
Copy link
Copy Markdown
Member Author

vrothberg commented Apr 16, 2020

@baude system tests are passing but I had to tweak container removal and ignore ErrNoSuchCtr at container sync. This is fishy and I am not sure if that patches a symptom rather than the cause. Did something change compared to v1 that would explain the need to ignore the error there? Cc @mheon

@mheon
Copy link
Copy Markdown
Member

mheon commented Apr 16, 2020

Wait, what? The container registers as present for HasContainer but it's gone by the time syncContainer() fires? If that's actually consistent, it sounds like something is wrong.

@vrothberg
Copy link
Copy Markdown
Member Author

vrothberg commented Apr 16, 2020 •
edited
Loading

Wait, what? The container registers as present for HasContainer but it's gone by the time syncContainer() fires? If that's actually consistent, it sounds like something is wrong.

Yes, that's the case. I could not reproduce manually but hit it 100 percent of times in the build system test. It screams race condition but so many things have changed that I have a hard time pinning it down.

@vrothberg
Copy link
Copy Markdown
Member Author

vrothberg commented Apr 16, 2020

As discussed, I dropped the patch, so we have a reproducer in in the build system test.

Ready to merge from my side.

jwhonce
jwhonce reviewed Apr 16, 2020
View reviewed changes
Comment thread cmd/podmanV2/build.go Outdated
@vrothberg
Copy link
Copy Markdown
Member Author

vrothberg commented Apr 16, 2020

Thanks, repushed!

rhatdan
rhatdan reviewed Apr 16, 2020
View reviewed changes
Comment thread cmd/podmanV2/utils/utils.go Outdated
Comment thread cmd/podmanV2/utils/utils.go Outdated
Comment thread libpod/runtime_ctr.go Outdated
@rh-atomic-bot
Copy link
Copy Markdown
Collaborator

rh-atomic-bot commented Apr 16, 2020

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

@vrothberg
podmanV2: implement build
60624f9 
Implement `podman build` for the local client.  The remote client will
require some rather large work in the backend and a new build endpoint
for the libpod rest API.

Signed-off-by: Valentin Rothberg <rothberg@redhat.com>
@vrothberg
Copy link
Copy Markdown
Member Author

vrothberg commented Apr 17, 2020

Repushed.

@rhatdan
Copy link
Copy Markdown
Member

rhatdan commented Apr 17, 2020

LGTM

@TomSweeneyRedHat
Copy link
Copy Markdown
Member

TomSweeneyRedHat commented Apr 17, 2020

Changes LGTM, but I didn't test them.

@rhatdan
Copy link
Copy Markdown
Member

rhatdan commented Apr 17, 2020

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 17, 2020
@openshift-merge-robot openshift-merge-robot merged commit a5884c0 into containers:master Apr 17, 2020
@github-actions 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 github-actions Bot locked as resolved and limited conversation to collaborators Sep 25, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@rhatdan rhatdan rhatdan left review comments

@mrunalp mrunalp Awaiting requested review from mrunalp

@TomSweeneyRedHat TomSweeneyRedHat Awaiting requested review from TomSweeneyRedHat

+1 more reviewer

@jwhonce jwhonce jwhonce left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@rhatdan rhatdan

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@vrothberg @openshift-ci-robot @rh-atomic-bot @mheon @rhatdan @TomSweeneyRedHat @jwhonce @openshift-merge-robot