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

buildah bud tests under podman-remote #9887

Merged
merged 1 commit into from
Jul 29, 2021
Merged

buildah bud tests under podman-remote #9887

merged 1 commit into from
Jul 29, 2021

Conversation

edsantiago
Copy link
Member

A huge ton of skips: podman-remote build just doesn't
really work at all. Lots of issues filed, a few more
left to go. Submitting this just to see how it does in CI.

Signed-off-by: Ed Santiago [email protected]

@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 Mar 30, 2021
@openshift-ci-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: edsantiago

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

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 30, 2021
@edsantiago
Copy link
Member Author

--arch/--os/--platform don't work. --log-rusage either. Will file issues for those later today.

@edsantiago edsantiago force-pushed the test_buildah_bud_with_remote branch 6 times, most recently from d1bb4a2 to b152dc4 Compare April 4, 2021 11:35
@edsantiago edsantiago force-pushed the test_buildah_bud_with_remote branch from b152dc4 to e2d1d97 Compare April 7, 2021 22:05
@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 11, 2021
@openshift-ci-robot
Copy link
Collaborator

@edsantiago: PR needs rebase.

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.

@edsantiago edsantiago mentioned this pull request Apr 27, 2021
Closed
@openshift-ci openshift-ci bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 28, 2021
@edsantiago edsantiago force-pushed the test_buildah_bud_with_remote branch 3 times, most recently from 114c41d to f385806 Compare May 6, 2021 17:05
@edsantiago edsantiago mentioned this pull request May 10, 2021
Merged
@edsantiago edsantiago force-pushed the test_buildah_bud_with_remote branch 2 times, most recently from 33c6d93 to 793d8b5 Compare May 11, 2021 13:11
@openshift-ci openshift-ci bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 20, 2021
@edsantiago edsantiago force-pushed the test_buildah_bud_with_remote branch from 793d8b5 to f43c804 Compare May 20, 2021 19:06
@openshift-ci openshift-ci bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 20, 2021
@edsantiago edsantiago force-pushed the test_buildah_bud_with_remote branch 4 times, most recently from 70f0631 to 3b758d3 Compare May 26, 2021 21:07
@rhatdan
Copy link
Member

rhatdan commented Jul 10, 2021

@edsantiago Any issues still blocking this?

@edsantiago
Copy link
Member Author

Yes. #10154 ("stream dropped") causes 3-5 flakes per run; we can't even dream of merging until that issue is fixed.

@edsantiago edsantiago force-pushed the test_buildah_bud_with_remote branch 5 times, most recently from 0e7f901 to 09f3b1d Compare July 15, 2021 14:18
@edsantiago edsantiago force-pushed the test_buildah_bud_with_remote branch from 09f3b1d to 1b0da61 Compare July 26, 2021 11:45
@edsantiago edsantiago mentioned this pull request Jul 26, 2021
Closed
@edsantiago edsantiago force-pushed the test_buildah_bud_with_remote branch from 1b0da61 to c893d42 Compare July 28, 2021 12:50
@edsantiago
Copy link
Member Author

OMG it passed!! (with #11067)

@edsantiago edsantiago force-pushed the test_buildah_bud_with_remote branch from c893d42 to af99f02 Compare July 28, 2021 16:33
@edsantiago
buildah bud tests under podman-remote
ec9dad7 
New functionality -- mostly in the diffs we apply to
buildah's helpers.bash -- to enable running buildah-bud
tests under podman-remote. The gist of it is, we start
a 'podman system service' before each test, and clean
it up on test exit.

Design decision: the diff file for helpers.bash is no
longer trailing-whitespace-clean: that ended up producing
diffs that git wouldn't apply, because in some cases
the whitespace is actually important. In order to pass CI,
we need to exclude this file from some checks.

Signed-off-by: Ed Santiago <[email protected]>
@edsantiago edsantiago force-pushed the test_buildah_bud_with_remote branch from af99f02 to ec9dad7 Compare July 28, 2021 20:16
@edsantiago edsantiago removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 28, 2021
@edsantiago
Copy link
Member Author

@containers/podman-maintainers, @containers/build-maintainers, I believe this is ready for review. It's more complicated than I would've liked: the big diffs to helpers.bash are more likely to cause conflicts each time we vendor in a new buildah. I don't have any good ideas on how to solve that.

Despite the complexity, I think this is worth having. This remote test has already caught a large number of podman-remote bugs, and I think it's likely to catch more in the future.

vrothberg
vrothberg reviewed Jul 29, 2021
View reviewed changes
Copy link
Member

@vrothberg vrothberg left a comment

Choose a reason for hiding this comment

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

LGTM
Thanks for doing this, Ed! Quite a herculean task.

@vrothberg
Copy link
Member

@edsantiago please drop the WIP: if you want to merge

@rhatdan
Copy link
Member

rhatdan commented Jul 29, 2021

LGTM
Is there stuff we could do on the Buildah side to make the breaking vendors less likely?

@edsantiago edsantiago changed the title WIP: buildah bud tests under podman-remote buildah bud tests under podman-remote Jul 29, 2021
@edsantiago
Copy link
Member Author

The only thing to do, really, is be careful when touching helpers.bash. But that's really hard, because real problems come up in buildah-land that can only be addressed by tweaking that file.

Where my mind keeps going is: eliminate the diff, put the podman and podman-remote support directly in the buildah repo itself, but that just seems loathsome: buildah really shouldn't know about the kludges done in podman CI. I'd like to do it by some hook- or plugin-based mechanism, but my explorations there just lead to more complexity.

@vrothberg
Copy link
Member

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jul 29, 2021
@openshift-merge-robot openshift-merge-robot merged commit 9c4b7f9 into containers:main Jul 29, 2021
@edsantiago edsantiago deleted the test_buildah_bud_with_remote branch July 29, 2021 12:36
edsantiago added a commit to edsantiago/libpod that referenced this pull request Nov 2, 2022
@edsantiago
buildah bud tests: better handling of remote
56fae7d 
When I first enabled buildah-bud tests under podman-remote (containers#9887),
I got one aspect all wrong: I added a podman-remote() helper function
to match the podman() one. Turns out it's never actually called,
even when $PODMAN_BINARY=podman-remote, because functions/aliases
don't work that way.

The way it works is, those few cases in which bud.bats runs
podman are not magically remapped to podman-remote, they use
the podman() function. That's where we need to check if
we're using podman-remote, and that's where we need to
remove the registry-and-rootdir options.

With this fix, we can reenable two previously-skipped bud tests.

Signed-off-by: Ed Santiago <[email protected]>
@edsantiago edsantiago mentioned this pull request Nov 2, 2022
Merged
@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.
Reviewers

@vrothberg vrothberg vrothberg left review comments

Assignees

@vrothberg vrothberg

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.
5 participants
@edsantiago @openshift-ci-robot @rhatdan @vrothberg @openshift-merge-robot