-
Notifications
You must be signed in to change notification settings - Fork 198
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
Add Farms to config #1586
Add Farms to config #1586
Conversation
@vrothberg @rhatdan PTAL |
Carrying over my comment from the Podman PR: I mean that it should not be part of the [engine] table but a new [buildfarm] table. Ideally, it should not be part of containers.conf at all but a proper database or a dedicated predictable path for root and rootless. machine and conenction are (ab)using containers.conf already as a database but there is growing desire to change that, see #1566. containers.conf is a config file for users and we somehow abuse it as a database. |
I agree this should at least be in a separate containers.conf file that can be updated on the fly. Moving everything to a dedicated connections.conf would be preferred to share both. |
I suggest to integrate buildfarm into the brainstorming in #1566. I think it's a bad idea to use a config file as a DB. BUT adding buildfarm as done in this PR will not change the status quo since we're already using containers.conf as a DB. It would be nice to find a more elegant solution in the future but we shouldn't block buildfarm's progress on this issue. |
I updated the PR to create a separate |
786ebcb
to
5197889
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Requires tests and we need to set defaults (i.e., create an empty map to avoid the nil checks).
3463236
to
bd68227
Compare
Tests are green! |
LGTM |
The Podman PR containers/podman#19358 doesn't pass CI yet. Let's wait to be sure this PR has everything containers/podman#19358 needs. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is missing documentation in the man page
Add two new fields Farms and DefaultFarm to the Config to be used by the new podman buildfarm command. Signed-off-by: Urvashi Mohnani <umohnani@redhat.com>
A new --farm flag is being added to podman system connection add so that when a new connection is added it can be added to a new or existing farm. Update the code here to be able to do that. Signed-off-by: Urvashi Mohnani <umohnani@redhat.com>
Added docs |
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: rhatdan, umohnani8, 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 |
Add 2 new fields to EngineConfig
Farms
andDefaultFarm
for the podman buildfarm feature work.Also update the ssh connection code to be able to add a new connection to a new or existing farm when the
--farm
is set inpodman system connection add
.PR containers/podman#19358 will vendor this in.