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.

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

[4.7] fix: check wsl npipe when executing podman compose #20491

Merged
merged 1 commit into from
Oct 27, 2023

Conversation

vrothberg
Copy link
Member

Does this PR introduce a user-facing change?

fix podman compose command not working with wsl

[NO NEW TESTS NEEDED]

Signed-off-by: lstocchi <[email protected]>
Cherry-picked-by: Valentin Rothberg <[email protected]>
@openshift-ci openshift-ci bot added release-note do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels Oct 26, 2023
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Oct 26, 2023

[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

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-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 26, 2023
@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 26, 2023
@openshift-merge-robot
Copy link
Collaborator

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.

@github-actions github-actions bot added the kind/api-change Change to remote API; merits scrutiny label Oct 26, 2023
@vrothberg vrothberg changed the base branch from main to v4.7 October 26, 2023 10:40
@vrothberg vrothberg marked this pull request as ready for review October 26, 2023 10:40
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 26, 2023
@vrothberg vrothberg changed the title Cherry pick 20478 to v4.7 [4.7] fix: check wsl npipe when executing podman compose Oct 26, 2023
@vrothberg
Copy link
Member Author

@cevich CI on this branch is sick

@Luap99
Copy link
Member

Luap99 commented Oct 26, 2023

@cevich CI on this branch is sick

Looks like #20341 was merged without passing tests.

But the better question is why do we even run validate on more than one disto/arch? Shouldn't one time be enough? Validate checks based on the source code so checking more than once seems wasteful.

@vrothberg
Copy link
Member Author

@baude shall we merge and get CI back later?

@TomSweeneyRedHat
Copy link
Member

Just adding the upstream PR for reference: #20478

@TomSweeneyRedHat
Copy link
Member

LGTM
I'd be fine with pushing the red merge button at this point while @cevich digs at the CI issue. I'll let @bbaude or @mheon make the final decision.

@cevich
Copy link
Member

cevich commented Oct 26, 2023

The only reason I can think of to run it across multiple platforms is to ensure it also functions if a developer runs it locally (on their random platform). Maybe we don't care. I'm fine if someone wants to strip out the aarch64 validate.

@mheon
Copy link
Member

mheon commented Oct 26, 2023

I think that the test definitely has value so long as we build for Macs, and we aren't planning on dropping that any time soon. Any idea what's going on - is it just this branch?

@mheon
Copy link
Member

mheon commented Oct 26, 2023

@Luap99 I think the multiple validates is mostly about build tags - we have a lot of things (esp. in machine) build-tagged for a single architecture or OS

@vrothberg
Copy link
Member Author

Can we get this in?

@cevich
Copy link
Member

cevich commented Oct 27, 2023

I think the multiple validates is mostly about build tags

Aaaahhh, that sounds like a good reason. Otherwise those files would all just be ignored.

In any case, the python failure here I'm clueless about. The pre-commit (python) check has no use across multiple arches (that I'm aware of). Unfortunately this is an item that does runtime installs as well, so...well...there you go.

It's been a long while since I looked at these CI codepaths, but IIRC they're all lumped in together under a single make target :S

@mheon
Copy link
Member

mheon commented Oct 27, 2023

Alright, I'm just going to press the button. I would say we should investigate further, but we only have to maintain this branch for another ~3 weeks, so it's probably not worth it.

@mheon mheon merged commit afb7d7e into containers:v4.7 Oct 27, 2023
@cevich
Copy link
Member

cevich commented Oct 27, 2023

maintain this branch for another ~3 weeks

Oh forget it, definitely not worth it. And we don't see this happening on main so 🤷‍♂️

@vrothberg vrothberg deleted the cherry-pick-20478-to-v4.7 branch October 27, 2023 15:13
@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 Jan 26, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 26, 2024
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. kind/api-change Change to remote API; merits scrutiny locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. release-note
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants