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

containers.no_hosts is not considered by podman system service #13725

Closed
wants to merge 1 commit into from

Conversation

reda-alaoui
Copy link

Fix #13719

Signed-off-by: Réda Housni Alaoui [email protected]

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Mar 30, 2022

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: reda-alaoui
To complete the pull request process, please assign saschagrunert after the PR has been reviewed.
You can assign the PR to them by writing /assign @saschagrunert in a comment when ready.

The full list of commands accepted by this bot can be found 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

@rhatdan
Copy link
Member

rhatdan commented Mar 30, 2022

You will need to update tests.

@reda-alaoui
Copy link
Author

reda-alaoui commented Mar 30, 2022

I am not fluent in Go. Could you point me to them?
FYI, I wanted to add tests calling directly the rest endpoint but didn’t find any example.

@rhatdan
Copy link
Member

rhatdan commented Mar 30, 2022

Most tests are under tests/
API Tests are in tests/apiv2

@reda-alaoui reda-alaoui marked this pull request as draft March 31, 2022 08:05
@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 Mar 31, 2022
@reda-alaoui reda-alaoui force-pushed the fix-13719 branch 3 times, most recently from ebe5b95 to 6f21568 Compare March 31, 2022 09:16
@reda-alaoui reda-alaoui marked this pull request as ready for review March 31, 2022 10:53
@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 Mar 31, 2022
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.

This should be fixed somewhere in specgen and not in the libpod container config.

The import rule is that cli flag must always overwrite the config setting, with your change this does not work because --no-hosts=false no longer overwrites the config setting, see the failing tests.

@reda-alaoui
Copy link
Author

@Luap99 I took inspiration from rhatdan@f40a0e7 .
But ok, I will take a look at specgen.

@reda-alaoui reda-alaoui marked this pull request as draft March 31, 2022 17:55
@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 Mar 31, 2022
@reda-alaoui
Copy link
Author

@Luap99 could you point me to an architecture overview document? I don't understand what is specgen and how it interacts with libpod.

@reda-alaoui
Copy link
Author

reda-alaoui commented Apr 2, 2022

In the end, the real issue for my case is #13748 . So giving up on this one.

@reda-alaoui reda-alaoui closed this Apr 2, 2022
@reda-alaoui reda-alaoui deleted the fix-13719 branch April 4, 2022 16:16
@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 21, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 21, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. 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.

containers.no_hosts is not considered by podman system service
3 participants