Refactor Podman E2E helpers to allow passing/adding more options to the low-level executor #25097
Conversation
... replacing the many parameters with a struct with named fields. This makes the meaning of parameters more explicit, and more importantly it makes it easier to just edit _one_ of the parameters without requiring specialized wrappers for every single case. Should not change behavior. Signed-off-by: Miloslav Trmač <mitr@redhat.com>
This will make it easier to structure the API, at the cost of making it a bit more opaque about which parts of PodmanExecOptions are implemented where. Should not change behavior. Signed-off-by: Miloslav Trmač <mitr@redhat.com>
Eliminate this helper / indirection, and pass around PodmanExecOptions explicitly. Should not change behavior. Signed-off-by: Miloslav Trmač <mitr@redhat.com>
Pass exactly the same PodmanExecOptions to makeOptions and to PodmanExecBaseWithOptions. This will allow simplifying the code further. Should not change behavior. Signed-off-by: Miloslav Trmač <mitr@redhat.com>
... and, to an extent, centralize the PodmanSessionIntegration creation in that function. This reduces duplication, and we will further eliminate some of the callers. Should not change behavior. Signed-off-by: Miloslav Trmač <mitr@redhat.com>
Instaed, inline the implementation into callers, calling PodmanWithOptions directly, demonstrating how to use PodmanWithOptions. Should not change behavior. Signed-off-by: Miloslav Trmač <mitr@redhat.com>
| } | ||
|
|
||
| // PodmanExecBaseWithOptions execs podman with the specified args, and in an environment defined by options | ||
| func (p *PodmanTest) PodmanExecBaseWithOptions(args []string, options PodmanExecOptions) *PodmanSession { |
There was a problem hiding this comment.
This name is fairly ridiculous … I considered reusing the now-available PodmanBase name, but I left it as is because it seems to me that most users should call something higher-level, and a long tedious name is a bit of a hint to use something else.
|
See individual commits for details — that’s probably a more convenient way to review. |
It seems this utility is not all that generally useful, so eliminate it from the global namespace and use PodmanWithOptions directly. Should not change behavior. Signed-off-by: Miloslav Trmač <mitr@redhat.com>
This is a generalization of PodmanExitCleanly, scalable to an arbitrary number of possible options. Should not change behavior. Signed-off-by: Miloslav Trmač <mitr@redhat.com>
|
lgtm thanks @mtrmac |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Luap99, mtrmac 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 |
|
/lgtm |
I’d be happy to do that, but I would be submitting a PR where half of the helpers get a comment saying “careful, mitr doesn’t understand why this exists / deviates”. |
This, primarily, replaces
PodmanAsUserBase(called with 9 parameters) withPodmanExecBaseWithOptionscalled withPodmanExecOptions, where the caller can only specify the relevant options, and by name; and it threadsPodmanExecOptionsthrough the call stack, so that E2E tests can set these options directly.This simplifies the call stack and eliminates the need for various helpers. My primary motivation is that, for #25007 (comment) , I want to add a “do not log Podman output to the Ginkgo log” option, and this will allow doing that without adding a one more (unnamed) boolean parameter, or perhaps a new set of helpers, throughout the call stack.
Tests can now, in addition to
Podman(), callPodmanWithOptions(), and also (following the theme of #24977 ),PodmanExitCleanlyWithOptions.And, in the future, maybe
PodmanExecOptionscould be extended with an expected exit code, or perhaps even an expected log output, making the test invocation even more declarative — but that’s not this PR.Warning: I don’t fully understand the current code.
makeOptionswhen the non-remote code relies on setting thePodmanMakeOptionsupcall (which also seems to be involved in the remote code?)common_test.goand intest/utils/utils.gobypass the difference above (e.g. don’t call.Podman(), and callPodmanAsUserBasedirectly and perhaps directly construct aPodmanSessionIntegrationThere is quite likely a deeper logic to this which I couldn’t figure out. Or maybe these are just inconsistencies accrued over time with copy&paste. This PR is intended to be a refactoring with no change to how tests operate; it doesn’t have the ambition to clean these questions up.
Does this PR introduce a user-facing change?