Add phase 1 of podman farm subcommands #19358

umohnani8 · 2023-07-25T15:19:50Z

This PR is phase 1 of adding the new podman farm subcommands.
Add farm create, list, remove, an update subcommands - complete with docs and tests.
Update the podman system connection add & remove commands to add and remove connections to and from farms - complete with docs and tests.

The podman farm command is currently hidden till it is fully ready, which will be in a follow up PR once this gets in.

Does this PR introduce a user-facing change?

Add podman farm create, list, remove, and update commands.
Update podman system connection add & remove to add and remove connections to and from farms.

umohnani8 · 2023-07-25T15:20:23Z

@rhatdan @vrothberg @mheon @nalind PTAL

vrothberg

Can we remove or comment-out those flags that are currently NOPs? Many flags being added have no test. If we add the functionality later on, it's easy to loose track of what's implemented and what not making reviews hard.

Please break future PRs into smaller commits. It's quite hard to review so much functionality at once. I'm mostly afraid to oversee things during review. Once released, it's hard to correct after the fact.

cmd/podman/buildfarm/create.go

cmd/podman/buildfarm/inspect.go

cmd/podman/buildfarm/list.go

cmd/podman/buildfarm/prune.go

cmd/podman/buildfarm/remove.go

test/e2e/buildfarm_test.go

vrothberg · 2023-07-26T07:52:20Z

test/e2e/buildfarm_test.go

+			Expect(session.Out.Contents()).Should(ContainSubstring("Farm \"farm3\" updated"))
+
+			// update farm2 to be the default farm
+			cmd = []string{"buildfarm", "update", "--default", "farm2"}


What if farm1 is already default and I do a buildfarm update --default=false farm1?

Good question - if that is the case we set DefaultFarm to "" so that none of the existing farms are set as the default.
Added a test case for this.

vendor/github.com/containers/common/pkg/config/config.go

umohnani8 · 2023-07-26T13:57:55Z

@vrothberg I broke the PR into as small commits as possible to help make reviews easier.

vrothberg · 2023-07-26T14:03:40Z

@vrothberg I broke the PR into as small commits as possible to help make reviews easier.

Thanks!

cmd/podman/system/connection/add.go

umohnani8 · 2023-08-02T15:04:22Z

So the man page and help menu scripts are unhappy because buildfarm is a hidden subcommand and doesn't show up in the help menu even though it is in the man pages. But if I remove it from the podman.1.md man page, the other script complains that buildfarm is not listed in podman.1.md - should I just not add man pages for this? Or is there another possible workaround? Fixing it for one of the scripts makes the other script unhappy https://github.com/containers/podman/pull/19358/checks?check_run_id=15557073962

Nvm, I found a way to ignore it as it is a hidden command.

docs/source/markdown/podman-buildfarm.1.md

cmd/podman/buildfarm/create.go

rhatdan · 2023-08-03T09:42:08Z

cmd/podman/buildfarm/create.go

+	}
+
+	if _, ok := cfg.Farms.List[farmName]; ok {
+		// if farm exists return an error


Should we add a --replace option?

we could, but we already have buildfarm update that let's you add or remove connections from a farm and buildfarm rm can be used to remove a farm completely to create a new one.
I don't mind adding it, just think there are already other ways of doing a replace.

I suggest to go ahead and implement the core functionality. We can always improve UX at a later point. At the moment, the core functionality of actually using buildfarm is missing and I think we need to make progress there first.

cmd/podman/buildfarm/create.go

rhatdan · 2023-08-03T09:54:20Z

cmd/podman/buildfarm/remove.go

+	}
+
+	for _, k := range deletedFarms {
+		fmt.Printf("Farm %q deleted\n", k)


Do we do this when we remove containers from pods? Or does it just silently exit.

We print the container ID when it is successfully removed

docs/source/markdown/podman-buildfarm-remove.1.md

cmd/podman/buildfarm/update.go

rhatdan · 2023-08-03T09:56:42Z

docs/source/markdown/podman-buildfarm-update.1.md

+
+## EXAMPLE
+```
+$ podman buildfarm update --add f35,f38 farm1


Should we have podman buildfarm add and podman buildfarm remove?

I think buildfarm update is a better option. I feel like add and create are very similar and can be confusing (I think they are basically the same thing for system connection). And buildfarm remove is already used for removing farms.

cmd/podman/system/connection/add.go

rhatdan · 2023-08-03T10:00:25Z

Very nice work.

cmd/podman/buildfarm/buildfarm.go

rhatdan · 2023-08-08T21:14:17Z

We agreed at the meeting today to sed 's/buildfarm/farm/g'

Vendor latest c/common with changes to add a new Farms table to containers.conf and update system connection to add a connection to a farm when --farm is set. Signed-off-by: Urvashi Mohnani <[email protected]>

Signed-off-by: Urvashi Mohnani <[email protected]>

umohnani8 · 2023-08-09T15:34:56Z

Moved from podman buildfarm to podman farm.

Signed-off-by: Urvashi Mohnani <[email protected]>

The podman farm create command allows users to create farms from the avaiable podman system connections. Signed-off-by: Urvashi Mohnani <[email protected]>

The podman farm list command allows users to list the existing farms. Signed-off-by: Urvashi Mohnani <[email protected]>

Podman farm remove allows users to remove one or more existing farms. Signed-off-by: Urvashi Mohnani <[email protected]>

Podman farm update allows users to update a farm by addig connections, removing connections, or changing the default farm. Signed-off-by: Urvashi Mohnani <[email protected]>

Add tests for podman farm create, remove, and update. Signed-off-by: Urvashi Mohnani <[email protected]>

Add new --farm flag to podman system connection add so that a user can add a new connection to a farm immediately. Update system connection remove such that when a connection is removed, the connection is also removed from any farms that have it. Add docs and tests for these changes. Signed-off-by: Urvashi Mohnani <[email protected]>

vrothberg

LGTM

openshift-ci · 2023-08-10T07:33:06Z

[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:

~~OWNERS~~ [rhatdan,umohnani8,vrothberg]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

rhatdan · 2023-08-10T13:23:31Z

/lgtm

openshift-ci bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. release-note approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jul 25, 2023

vrothberg reviewed Jul 26, 2023

View reviewed changes

umohnani8 force-pushed the buildfarm-2 branch from a9e85c7 to 6b92f2d Compare July 26, 2023 13:52

umohnani8 mentioned this pull request Jul 26, 2023

Add Farms to config containers/common#1586

Merged

umohnani8 force-pushed the buildfarm-2 branch from 6b92f2d to 5f9e5ab Compare August 1, 2023 16:05

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 1, 2023

umohnani8 force-pushed the buildfarm-2 branch from 5f9e5ab to 28537e6 Compare August 1, 2023 16:09

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 1, 2023

umohnani8 force-pushed the buildfarm-2 branch 4 times, most recently from 660f34e to c31cd12 Compare August 1, 2023 18:31

vrothberg reviewed Aug 2, 2023

View reviewed changes

cmd/podman/system/connection/add.go Show resolved Hide resolved

umohnani8 force-pushed the buildfarm-2 branch 5 times, most recently from a6f4fae to 2f4921c Compare August 2, 2023 14:23

umohnani8 force-pushed the buildfarm-2 branch from 2f4921c to 9e42b88 Compare August 2, 2023 15:11

umohnani8 changed the title ~~[WIP] Add phase 1 of podman buildfarm subcommands~~ Add phase 1 of podman buildfarm subcommands Aug 2, 2023

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 Aug 2, 2023

rhatdan approved these changes Aug 3, 2023

View reviewed changes

umohnani8 force-pushed the buildfarm-2 branch from 9e42b88 to 0b562b4 Compare August 3, 2023 15:39

umohnani8 force-pushed the buildfarm-2 branch from 0b562b4 to 8a87df5 Compare August 8, 2023 14:49

vrothberg reviewed Aug 8, 2023

View reviewed changes

cmd/podman/buildfarm/buildfarm.go Outdated Show resolved Hide resolved

umohnani8 added 2 commits August 9, 2023 11:07

Vendor c/common changes

b672ddc

Vendor latest c/common with changes to add a new Farms table to containers.conf and update system connection to add a connection to a farm when --farm is set. Signed-off-by: Urvashi Mohnani <[email protected]>

Add completion for Farms

2951316

Signed-off-by: Urvashi Mohnani <[email protected]>

umohnani8 force-pushed the buildfarm-2 branch from 8a87df5 to a41a958 Compare August 9, 2023 15:31

umohnani8 changed the title ~~Add phase 1 of podman buildfarm subcommands~~ Add phase 1 of podman farm subcommands Aug 9, 2023

umohnani8 added 7 commits August 9, 2023 13:37

Add podman farm subcommand

1d5ef29

Signed-off-by: Urvashi Mohnani <[email protected]>

Add podman farm create command

6fcdf1a

The podman farm create command allows users to create farms from the avaiable podman system connections. Signed-off-by: Urvashi Mohnani <[email protected]>

Add podman farm list command

59a8140

The podman farm list command allows users to list the existing farms. Signed-off-by: Urvashi Mohnani <[email protected]>

Add podman farm remove command

44a704d

Podman farm remove allows users to remove one or more existing farms. Signed-off-by: Urvashi Mohnani <[email protected]>

Add podman farm update command

dce3ef3

Podman farm update allows users to update a farm by addig connections, removing connections, or changing the default farm. Signed-off-by: Urvashi Mohnani <[email protected]>

Add tests for podman farm

310f971

Add tests for podman farm create, remove, and update. Signed-off-by: Urvashi Mohnani <[email protected]>

umohnani8 force-pushed the buildfarm-2 branch from a41a958 to bcebcad Compare August 9, 2023 17:37

vrothberg approved these changes Aug 10, 2023

View reviewed changes

openshift-ci bot assigned rhatdan Aug 10, 2023

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Aug 10, 2023

openshift-merge-robot merged commit 14e290a into containers:main Aug 10, 2023

afbjorklund mentioned this pull request Sep 29, 2023

farm command not listed #20207

Closed

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 Nov 9, 2023

github-actions bot locked as resolved and limited conversation to collaborators Nov 9, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add phase 1 of podman farm subcommands #19358

Add phase 1 of podman farm subcommands #19358

umohnani8 commented Jul 25, 2023 •

edited

Loading

umohnani8 commented Jul 25, 2023

vrothberg left a comment

vrothberg Jul 26, 2023

umohnani8 Jul 26, 2023

umohnani8 commented Jul 26, 2023

vrothberg commented Jul 26, 2023

umohnani8 commented Aug 2, 2023 •

edited

Loading

rhatdan Aug 3, 2023

umohnani8 Aug 3, 2023

vrothberg Aug 4, 2023

rhatdan Aug 3, 2023

umohnani8 Aug 3, 2023

rhatdan Aug 3, 2023

umohnani8 Aug 3, 2023

vrothberg Aug 10, 2023

rhatdan commented Aug 3, 2023

rhatdan commented Aug 8, 2023

umohnani8 commented Aug 9, 2023

vrothberg left a comment

openshift-ci bot commented Aug 10, 2023

rhatdan commented Aug 10, 2023

Add phase 1 of podman farm subcommands #19358

Add phase 1 of podman farm subcommands #19358

Conversation

umohnani8 commented Jul 25, 2023 • edited Loading

Does this PR introduce a user-facing change?

umohnani8 commented Jul 25, 2023

vrothberg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

umohnani8 commented Jul 26, 2023

vrothberg commented Jul 26, 2023

umohnani8 commented Aug 2, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rhatdan commented Aug 3, 2023

rhatdan commented Aug 8, 2023

umohnani8 commented Aug 9, 2023

vrothberg left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Aug 10, 2023

rhatdan commented Aug 10, 2023

umohnani8 commented Jul 25, 2023 •

edited

Loading

umohnani8 commented Aug 2, 2023 •

edited

Loading