Skip to content

Conversation

@cevich
Copy link
Member

@cevich cevich commented Dec 4, 2018

Building/installing dependencies from fixed source-version ensures
testing is reliable, but introduces a maintenance burden and
risks testing far outside of a real-world environment. The
sensible alternative is to install dependencies from distro-packaging
systems.

Install all development and testing dependencies at cache-image build
time, to help ensure testing remains stable. The existing cache-image
build workflow can be utilized at any future time to build/test
with updated packages.

@openshift-ci-robot openshift-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Dec 4, 2018
@cevich cevich changed the title Cirrus: Use packaged dependencies WIP: [skip ci] Cirrus: Use packaged dependencies Dec 4, 2018
@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 Dec 4, 2018
@cevich cevich changed the title WIP: [skip ci] Cirrus: Use packaged dependencies WIP: Cirrus: Use packaged dependencies Dec 4, 2018
@cevich cevich force-pushed the cirrus_packaged_deps branch 5 times, most recently from 452bc54 to c65ff39 Compare December 4, 2018 19:54
@rh-atomic-bot
Copy link
Collaborator

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

@cevich cevich force-pushed the cirrus_packaged_deps branch 15 times, most recently from caef081 to e778929 Compare December 5, 2018 21:29
@cevich
Copy link
Member Author

cevich commented Dec 5, 2018

N/B: Images built from this PR here: https://cirrus-ci.com/task/6236613032017920

@cevich cevich force-pushed the cirrus_packaged_deps branch from e778929 to a29ff52 Compare December 5, 2018 22:32
@cevich
Copy link
Member Author

cevich commented Dec 6, 2018

Status (Both Fedora 29 and Ubuntu 18.04)

  • The package-based mechanics are working (though may be missing some pieces).
  • Direct podman testing dependencies are installed/upgraded at cache-image build-time.
  • At PR test-time, they are installed/upgraded again (to handle differences from, or benefit from cache).
  • All files associated with the 'podman' package are removed (to prepare building/installing from source).

@baude @mheon need some discussion/guidance here, given the frequent "issues" we've hit in PR & Master testing WRT runc (or any other direct dependency really). I'm afraid this approach is going to shackle modern libpod development work, to potentially much older dependency (e.g. runc) API's, behaviors, and bugs. At the same time, for any expected incompatible feature/API gaps, specific tests will need to be automatically skipped based on the dependency versions. That often leads to incomplete code coverage at the "edges" of tests.

In other words, if a runc problem is found by CI in a libpod PR, the author may potentially wait a long time (days) for an upstream fix, test, review, merge, test, package, and more testing.

Or (maybe worse), a test which touches a new (not yet available in runc package) feature and other code may be skipped. Meaning the "other" code does not get tested at all.

Are these things actual problems, and something we want to stand in front of as a libpod PR merge requirement? Tell me your thoughts?

BTW: I'm open to having two sets of tests, one being package-based, the other running with test-time compiled, "latest-master" version of the direct-dependencies (no sha-binding like before). If this is a better idea, which (or both) should be a gate for PR merges vs just advisory testing?

@cevich cevich force-pushed the cirrus_packaged_deps branch from e8471cd to 654744c Compare June 25, 2019 15:20
@openshift-ci-robot openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 25, 2019
@cevich
Copy link
Member Author

cevich commented Jun 25, 2019

note to me: Ubuntu fails b/c searching for conmon in deprecated locations. Need something like #3429 to fix.

This was referenced Jun 25, 2019
@cevich cevich force-pushed the cirrus_packaged_deps branch from 654744c to cad7137 Compare June 26, 2019 19:32
Building/installing dependencies from fixed source-version ensures
testing is reliable, but introduces a maintenance burden and
risks testing far outside of a real-world environment.  The
sensible alternative is to install dependencies from distro-packaging
systems.

Install all development and testing dependencies at VM cache-image build
time, to help ensure testing remains stable.  The existing cache-image
build workflow can be utilized at any future time to build/test
with updated packages.

***N/B***: This does not update any dockerfiles used by testing, that is
left up to future efforts.

Signed-off-by: Chris Evich <[email protected]>
@cevich cevich force-pushed the cirrus_packaged_deps branch from cad7137 to 3d559df Compare June 27, 2019 13:07
@cevich cevich changed the title WIP: Cirrus: Use packaged-based dependencies Cirrus: Use packaged-based dependencies Jun 27, 2019
@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 Jun 27, 2019
@cevich
Copy link
Member Author

cevich commented Jul 1, 2019

@rhatdan @mheon @baude Finally after months and months of toil, I think this is ready. PTAL.

@mheon
Copy link
Member

mheon commented Jul 1, 2019

This is smaller than I thought it would be. A lot easier to review as such.

Initial pass, everything seems reasonable. I'd just like @baude to have a look at the Varlink stuff - I know that's a packaging minefield.

@cevich
Copy link
Member Author

cevich commented Jul 1, 2019

This is smaller than I thought it would be.

Much of what made this PR so large originally, was split off into XXXL #2561 (already merged) 😄 Having that was absolutely critical for finishing development on this PR. Also critical was all of @haircommander and @lsm5 efforts on splitting/packaging conmon and maintaining the other dependent packages (thanks!).

@cevich
Copy link
Member Author

cevich commented Jul 8, 2019

@mheon @baude anything else holding this one back?

@mheon
Copy link
Member

mheon commented Jul 8, 2019

LGTM on my end
@baude @rhatdan @giuseppe PTAL

@rhatdan
Copy link
Member

rhatdan commented Jul 8, 2019

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 8, 2019
@openshift-merge-robot openshift-merge-robot merged commit 7633bd3 into containers:master Jul 8, 2019
cevich added a commit to cevich/podman that referenced this pull request Jul 11, 2019
This was originally intended, but somehow omitted from containers#1936

Signed-off-by: Chris Evich <[email protected]>
cevich added a commit to cevich/podman that referenced this pull request Jul 12, 2019
This was originally intended, but somehow omitted from containers#1936

Signed-off-by: Chris Evich <[email protected]>
@cevich cevich deleted the cirrus_packaged_deps branch June 30, 2021 18:11
@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 22, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 22, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

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

Development

Successfully merging this pull request may close these issues.

8 participants