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

Enable win podman-machine test failure #20852

Merged
merged 1 commit into from
Dec 3, 2023
Merged

Enable win podman-machine test failure #20852

merged 1 commit into from
Dec 3, 2023

Conversation

cevich
Copy link
Member

@cevich cevich commented Nov 30, 2023 •
edited
Loading

Intended to serve as motivation to fix them. Removed from status
aggregator so the failures don't block PR merging. Updated comment text
to reference related open issue, #20548.

Does this PR introduce a user-facing change?

None

@cevich cevich marked this pull request as draft November 30, 2023 15:28
@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 30, 2023
@cevich
Copy link
Member Author

cevich commented Nov 30, 2023

Re-drafting this. After discussing with @mheon he suggested making the test red, but removing from the Total Success status aggregator. Making that change now...

@cevich cevich changed the title [CI:DOCS] Update comment w/ issue ref. Enable win podman-machine test failure Nov 30, 2023
@cevich cevich requested a review from mheon November 30, 2023 15:34
@mheon
Copy link
Member

mheon commented Nov 30, 2023

LGTM. This is going to suck but let's tear off the band-aid and stop Windows regressions

@cevich cevich mentioned this pull request Nov 30, 2023
Closed
@cevich cevich marked this pull request as ready for review November 30, 2023 15:37
@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 Nov 30, 2023
@cevich
Enable win podman-machine test failure
f9e8585 
Intended to serve as motivation to fix them.  Removed from status
aggregator so the failures don't block PR merging.  Updated comment text
to reference related open issue, containers#20548.

Signed-off-by: Chris Evich <[email protected]>
@cevich cevich mentioned this pull request Nov 30, 2023
Merged
@baude
Copy link
Member

baude commented Nov 30, 2023

@cevich quick work! I don't think this addresses #20843 however .. is that something that can be included in this PR so we can work on both WSL and HyperV?

@Luap99
Copy link
Member

Luap99 commented Nov 30, 2023

https://cirrus-ci.com/task/6103111505281024 is still green despite test errors in the log so I do not think this is working as interned?

@cevich
Copy link
Member Author

cevich commented Nov 30, 2023

@baude Didn't see that issue, ugh, that stinks. Besides permissions, the "session" the tests run in matters also. I don't actually understand this stuff...but the contrib/cirrus/win-sess-launch.ps1 is suppose to deal with the session being owned by SYSTEM vs ADMINISTRATOR. Maybe this is involved in #20843, maybe not, I can only guess 😕

Re: is still green
Thanks for point this out Paul, I hadn't been watching closely. That's a problem I struggled with during implementation, and clearly it wasn't fixed or re-broke. Windows is really stupid about it's error-handling, so I'm not all that surprised at this. I'll try and find some time to poke at it...

@cevich cevich mentioned this pull request Nov 30, 2023
Merged
@cevich
Copy link
Member Author

cevich commented Nov 30, 2023

...poke applied. I g2g though, so I'll check on it later.

@rhatdan
Copy link
Member

rhatdan commented Nov 30, 2023

/approve
LGTM

@openshift-ci OpenShift CI
Copy link
Contributor

openshift-ci bot commented Nov 30, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cevich, rhatdan

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 openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 30, 2023
@cevich
Copy link
Member Author

cevich commented Dec 1, 2023

Oof, so #20857 is going to need more work. I made it a separate PR in case this PR risks holding back the high-priority #20691 (since it depends on this one)

@cevich
Copy link
Member Author

cevich commented Dec 1, 2023

Confirmed #20857 fixes the pass-on-error problem.

@rhatdan
Copy link
Member

rhatdan commented Dec 3, 2023

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Dec 3, 2023
@openshift-merge-bot openshift-merge-bot bot merged commit 1672318 into containers:main Dec 3, 2023
93 checks passed
@Luap99 Luap99 mentioned this pull request Dec 11, 2023
Closed
@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 Mar 3, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 3, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@mheon mheon Awaiting requested review from mheon

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. release-note-none
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@cevich @mheon @baude @Luap99 @rhatdan