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

Resolve symlink path for qemu directory if possible #17027

Merged
merged 1 commit into from
Feb 8, 2023

Conversation

n8henrie
Copy link
Contributor

@n8henrie n8henrie commented Jan 7, 2023

Fixes #17026
Fixes NixOS/nixpkgs#169118

Related: NixOS/nixpkgs#163015

Does this PR introduce a user-facing change?

None

@n8henrie n8henrie force-pushed the issue_17026 branch 2 times, most recently from 3c9ae0e to cbddee8 Compare January 7, 2023 23:22
@n8henrie
Copy link
Contributor Author

n8henrie commented Jan 7, 2023

Please write a regression test for what you're fixing. Even if it
seems trivial or obvious, try to add a test that will prevent
regressions.

Adding [NO NEW TESTS NEEDED] as I don't see any related existing tests, the only change is following symlinks on darwin (not fedora).

However, an analogous change may be useful for nix users of aarch64-linux -- I don't know, will experiment.

@n8henrie
Copy link
Contributor Author

@rhatdan -- it seems that @Luap99 thinks this PR belongs in podman, and I think it makes sense here as well (at least for now -- going forward, if similar issues pop up for other projects or architectures it could be adopted for them as well).

As shown here, this patch seems to work well and would make a big difference for podman users on aarch64-darwin that are using nix (as opposed to homebrew) for package management, which I think is a growing group.

I'm not sure what's going on with those test failures though. My only code change was in pkg/machine/qemu/options_darwin_arm64.go, so I have to assume the windows failure is unrelated.

@rhatdan
Copy link
Member

rhatdan commented Feb 1, 2023

This is fine with me.

@n8henrie
Copy link
Contributor Author

n8henrie commented Feb 4, 2023

@rhatdan thanks for your help and feedback with this issue. It looks like you're in the list of approvers. Is this blocked on anything else from my side?

It looks like the CI failure is probably unrelated:

Error: copying system image from manifest list: ... dial tcp: lookup cdn03.quay.io: no such host

@Luap99
Copy link
Member

Luap99 commented Feb 6, 2023

@n8henrie Can you please rebase? While it is not required we often have issues with older PRs that where not fully rebased on main.

I will merge it then.

@n8henrie
Copy link
Contributor Author

n8henrie commented Feb 7, 2023

@Luap99 done, thanks!

Copy link
Member

@Luap99 Luap99 left a comment

Choose a reason for hiding this comment

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

LGTM

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Feb 7, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Luap99, n8henrie

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 Feb 7, 2023
@Luap99
Copy link
Member

Luap99 commented Feb 8, 2023

/lgtm
/cherry-pick v4.4

@openshift-cherrypick-robot
Copy link
Collaborator

@Luap99: once the present PR merges, I will cherry-pick it on top of v4.4 in a new PR and assign it to you.

In response to this:

/lgtm
/cherry-pick v4.4

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.

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Feb 8, 2023
@openshift-merge-robot openshift-merge-robot merged commit c5bfacd into containers:main Feb 8, 2023
@openshift-cherrypick-robot
Copy link
Collaborator

@Luap99: new pull request created: #17429

In response to this:

/lgtm
/cherry-pick v4.4

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 locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Sep 10, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 10, 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. release-note-none
Projects
None yet
6 participants