Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Refactor API build endpoint to be more compliant #7452

Merged
merged 1 commit into from
Sep 15, 2020
Merged

Refactor API build endpoint to be more compliant #7452

merged 1 commit into from
Sep 15, 2020

Conversation

jwhonce
Copy link
Member

@jwhonce jwhonce commented Aug 25, 2020 •
edited
Loading

  • Refactor channel.WriteCloser() to encapsulate the channel
  • Refactor endpoint to "live" stream buildah output channels over API
    rather then buffering output
  • Refactor bindings/tunnel build code for endpoint changes
  • Cleanup initiating extra image engine
  • Restore build system tests

Fixes #7136
Fixes #7137
Fixes #7602

Signed-off-by: Jhon Honce [email protected]

@jwhonce jwhonce added the kind/bug Categorizes issue or PR as related to a bug. label Aug 25, 2020
@jwhonce jwhonce self-assigned this Aug 25, 2020
@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 25, 2020
@rhatdan
Copy link
Member

rhatdan commented Aug 26, 2020

Can you either add tests or turn on remote tests to guarantee the two referenced issues, are permanently fixed.

@edsantiago
Copy link
Member

There are lots of disabled tests in test/system/070-build.bats. You can search for 7136/7137, remove those skips, and run via:

$ PODMAN=$(pwd)/bin/podman-remote bats test/system/070-build.bats

(setting up a system service is left as an exercise for the reader).

As of this writing, the tests all fail. One common factor is that the new code is not spitting out a COMMIT line (my tests expect to see COMMIT). There's another few weird errors, I'll let you sort those out.

@TomSweeneyRedHat
Copy link
Member

Lint is not your friend atm @jwhonce

@jwhonce jwhonce force-pushed the issues/7136 branch 2 times, most recently from afdc2c3 to 560c01e Compare August 27, 2020 17:31
@jwhonce
Copy link
Member Author

jwhonce commented Aug 27, 2020

@rhatdan @edsantiago Remote tests restored.

@edsantiago
Copy link
Member

@jwhonce you might want to rebase to pick up @mheon's fix for 7195. I'm not saying CI will fail, but yeah, I'm saying CI will likely fail. Sounds like a mafia threat.

@edsantiago
Copy link
Member

Tests are unhappy. I'm seeing a spurious $PACKER_BASE/fedora_packaging.sh && (and similar strings) which closely match other stuff I'm reviewing on another repo, so now I don't know if my browser is corrupt. Are you seeing those strings too in the build test logs? Anyhow, build tests are failing.

@edsantiago
Copy link
Member

ok this is eerie. The string bash $PACKER_BASE/fedora_packaging.sh appears in https://github.com/jwhonce/libpod/blob/0975f694c390f7b7f72a46493be081a1ee895d74/contrib/cirrus/packer/fedora_setup.sh#L22

How can that be interfering with a podman build ??

@mheon mheon mentioned this pull request Aug 29, 2020
Closed
@jwhonce jwhonce marked this pull request as draft August 31, 2020 16:46
@openshift-ci-robot 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 Aug 31, 2020
@jwhonce jwhonce force-pushed the issues/7136 branch 4 times, most recently from 6b31a76 to 9a7ca23 Compare August 31, 2020 22:47
@mheon mheon mentioned this pull request Sep 8, 2020
Merged
@vrothberg vrothberg mentioned this pull request Sep 8, 2020
Closed
@mheon mheon mentioned this pull request Sep 8, 2020
Closed
@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 11, 2020
@openshift-ci-robot openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 11, 2020
@jwhonce jwhonce force-pushed the issues/7136 branch 5 times, most recently from 873fdf7 to d166442 Compare September 14, 2020 17:12
edsantiago
edsantiago suggested changes Sep 14, 2020
View reviewed changes
Copy link
Member

@edsantiago edsantiago left a comment

Choose a reason for hiding this comment

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

Spelling nit, and two suggestions for tests

test/system/070-build.bats Outdated Show resolved Hide resolved
test/system/070-build.bats Outdated Show resolved Hide resolved
test/system/070-build.bats Outdated Show resolved Hide resolved
@edsantiago edsantiago mentioned this pull request Sep 14, 2020
Closed
@jwhonce
Refactor API build endpoint to be more compliant
146c68f 
* Refactor/Rename channel.WriteCloser() to encapsulate the channel
* Refactor build endpoint to "live" stream buildah output channels
  over API rather then buffering output
* Refactor bindings/tunnel build because endpoint changes
  * building tar file now in bindings rather then depending on
    caller
* Cleanup initiating extra image engine
* Remove setting fields to zero values (less noise in code)
* Update tests to support remote builds

Fixes containers#7136
Fixes containers#7137

Signed-off-by: Jhon Honce <[email protected]>
@jwhonce jwhonce marked this pull request as ready for review September 14, 2020 20:47
@openshift-ci-robot openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 14, 2020
@openshift-ci-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: edsantiago, jwhonce

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

The pull request process is described here

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

ashley-cui
ashley-cui reviewed Sep 14, 2020
View reviewed changes
Copy link
Member

@ashley-cui ashley-cui left a comment

Choose a reason for hiding this comment

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

Tested against #7602, LGTM

@rhatdan
Copy link
Member

rhatdan commented Sep 15, 2020

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 15, 2020
@openshift-merge-robot openshift-merge-robot merged commit 3b4ad9a into containers:master Sep 15, 2020
@edsantiago edsantiago mentioned this pull request Sep 15, 2020
Closed
@jwhonce jwhonce deleted the issues/7136 branch June 30, 2021 16:10
@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 23, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 23, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@ashley-cui ashley-cui ashley-cui left review comments

@TomSweeneyRedHat TomSweeneyRedHat TomSweeneyRedHat left review comments

@edsantiago edsantiago edsantiago approved these changes

@baude baude Awaiting requested review from baude

@giuseppe giuseppe Awaiting requested review from giuseppe

@mrunalp mrunalp Awaiting requested review from mrunalp

Assignees

@jwhonce jwhonce

@rhatdan rhatdan

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. kind/bug Categorizes issue or PR as related to a bug. 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
7 participants
@jwhonce @rhatdan @edsantiago @TomSweeneyRedHat @openshift-ci-robot @ashley-cui @openshift-merge-robot