POC: Replace given seeds with direct ipa calls and cleanup

[veronnicka]

The cleanup uses a Before hook, this runs before each scenario. In this hook, a cy.task is run in order to paralelize the deletion calls. I also had to run kinit before both deletion or adding elements.

The seed adds a new cy.ipa calls which runs podman exec webui ipa via cy.exec(). The seed call has to be inside the scenarios, in the begining, otherwise the cleanup call would delete it.

The integration tests are failing now because of the seed calls mentioned above. This will be corrected in a new PR that will implement the changes thoroughly for all the tests.

Summary by Sourcery

Replace UI-driven Cypress setup steps with direct IPA CLI calls and introduce a reusable IPA command helper and test environment cleanup hook.

New Features:

• Add a custom Cypress ipa command to run IPA CLI operations inside the web UI container.
• Introduce a global cleanup hook that removes most IPA test data before scenarios run, with special handling for core services.

Enhancements:

• Refactor various Given "... exists" steps to create resources via IPA CLI instead of navigating the UI flows.
• Extend Cypress type definitions to include the new ipa command for better TypeScript support.

[sourcery-ai]

Hey there - I've reviewed your changes - here's some feedback:

• The ipa command helper always appends a quoted name argument, which results in a literal "undefined" being passed when name is omitted (e.g. in cleanup); consider only appending the name segment if it is defined to avoid sending spurious arguments to ipa.
• The cleanup utility in cleanupHook.ts currently has the actual deletion loop commented out and only logs what would be deleted; either re-enable the deletion or remove the unused code to avoid confusion about whether cleanup is actually happening.
• The echoToTerminal helper writes to a hard-coded absolute path under /home/vmiticka/...; consider using a project-relative path or a configurable location (e.g. via Cypress config or environment) so the tests work on other machines and CI environments.

Prompt for AI Agents

Please address the comments from this code review:

## Overall Comments
- The `ipa` command helper always appends a quoted `name` argument, which results in a literal `"undefined"` being passed when `name` is omitted (e.g. in `cleanup`); consider only appending the name segment if it is defined to avoid sending spurious arguments to `ipa`.
- The `cleanup` utility in `cleanupHook.ts` currently has the actual deletion loop commented out and only logs what would be deleted; either re-enable the deletion or remove the unused code to avoid confusion about whether cleanup is actually happening.
- The `echoToTerminal` helper writes to a hard-coded absolute path under `/home/vmiticka/...`; consider using a project-relative path or a configurable location (e.g. via Cypress config or environment) so the tests work on other machines and CI environments.

## Individual Comments

### Comment 1
<location> `cypress/support/commands.ts:44` </location>
<code_context>
+    name?: string,
+    options?: Partial<Cypress.ExecOptions>
+  ): Cypress.Chainable<Cypress.Exec> => {
+    // Ensure any quotes in the name are escaped to avoid breaking the shell
+    const safeName = name?.replace(/"/g, '\\"');
+    const cmd = `podman exec webui ipa ${subCommand} "${safeName}"`;
</code_context>

<issue_to_address>
**issue (bug_risk):** Avoid passing the string "undefined" as an argument when no name is provided to cy.ipa

Here, when `name` is omitted (e.g. `cy.ipa(command, undefined, ...)` in `cleanup`), `safeName` becomes the literal string `"undefined"`, so the command becomes `podman exec webui ipa <subCommand> "undefined"`. This changes the IPA call semantics and may break commands intended to run with no positional argument. Please build `cmd` without the name by default (e.g. `podman exec webui ipa ${subCommand}`) and only append `"${safeName}"` when `name` is actually provided.
</issue_to_address>

### Comment 2
<location> `cypress/e2e/common/cleanupHook.ts:33-35` </location>
<code_context>
+      `about to delete ${items.length} item(s): ${sample.join(", ")}${suffix}`
+    );
+
+    //items.forEach((u) => {
+    // cy.ipa(commands_delete.join(" "), u, { failOnNonZeroExit: false });
+    // });
+
+    echoToTerminal(
</code_context>

<issue_to_address>
**question (bug_risk):** The cleanup helper currently never deletes anything because the deletion loop is commented out

`cleanup` is only logging and not actually deleting, since the `items.forEach` loop is commented out. As a result, the `cleanup([...])` calls in the `Before` hook don’t clear state and may cause flaky tests. If this isn’t an intentional dry-run mode, please restore the deletion loop (with error handling as needed), or otherwise make the dry-run behavior explicit (e.g., a flag or separate helper).
</issue_to_address>

### Comment 3
<location> `cypress/e2e/common/cleanupHook.ts:3-8` </location>
<code_context>
+
+const echoToTerminal = (message: string) => {
+  const escaped = message.replace(/"/g, '\\"');
+  cy.exec(
+    `echo "${escaped}" >> /home/vmiticka/Programming/freeipa-webui/cypress/support/logs.log`
+  );
+};
</code_context>

<issue_to_address>
**suggestion (bug_risk):** Avoid hardcoding an absolute, user-specific path for logging

This absolute path is tied to a single machine and will fail or misbehave on CI and other developers’ environments. Please write the log to a project-relative path (e.g. `cypress/support/logs.log`) or derive it from a configurable environment variable so it works across setups.

```suggestion
const echoToTerminal = (message: string) => {
  // Allow overriding the log file location via Cypress env, but default to a project-relative path
  const logFilePath =
    (typeof Cypress !== "undefined" && Cypress.env("LOG_FILE_PATH")) ||
    "cypress/support/logs.log";

  const escapedMessage = message.replace(/"/g, '\\"');
  const escapedPath = logFilePath.replace(/"/g, '\\"');

  cy.exec(`echo "${escapedMessage}" >> "${escapedPath}"`);
};
```
</issue_to_address>

### Comment 4
<location> `cypress/e2e/common/cleanupHook.ts:10` </location>
<code_context>
+  );
+};
+
+export const cleanup = (
+  commands_find: string[],
+  commands_delete: string[],
</code_context>

<issue_to_address>
**issue (complexity):** Consider refactoring the cleanup utilities into a more intention‑revealing, data‑driven design with centralized logging to remove duplication and make the test setup easier to maintain.

You can reduce complexity and repetition here without changing behavior by tightening the API and making the `Before` block data‑driven.

### 1. Simplify `cleanup`’s API

Right now callers need to know and repeat the full `*-find` and `*-del` commands:

```ts
cleanup(["group-find", "--pkey-only"], ["group-del"], ["admins", ...]);
```

You can keep the same functionality with a more intention‑revealing API that constructs the commands internally:

```ts
type CleanupOptions = {
  omit?: string[];
  // Optional: custom filter for items to keep
  keep?: (value: string) => boolean;
  // Optional: extra flags for the find call
  findFlags?: string[];
};

export const cleanup = (name: string, options: CleanupOptions = {}) => {
  const { omit = [], keep, findFlags = ["--pkey-only"] } = options;

  const findCmd = [`${name}-find`, ...findFlags].join(" ");
  const deleteCmd = `${name}-del`;

  echoToTerminal(
    `cleanup starting: find=[${findCmd}] delete=[${deleteCmd}] omit=[${omit}]`
  );

  cy.ipa(findCmd, undefined, { failOnNonZeroExit: false }).then((result) => {
    const items = result.stdout
      .split("\n")
      .map((line) => line.split(":")[1]?.trim() || "")
      .filter((u) => u.length > 0)
      .filter((u) => !omit.includes(u))
      .filter((u) => (keep ? keep(u) : true));

    const sample = items.slice(0, 10);
    const suffix = items.length > 10 ? " ..." : "";

    echoToTerminal(
      `about to delete ${items.length} ${name}(s): ${sample.join(", ")}${suffix}`
    );

    // items.forEach((u) => {
    //   cy.ipa(deleteCmd, u, { failOnNonZeroExit: false });
    // });

    echoToTerminal(`done cleanup: delete=[${deleteCmd}] total=${items.length}`);
  });
};
```

This keeps behavior but removes the need to build command arrays at each callsite and makes misuse harder.

### 2. Generalize `cleanupServices` using `cleanup`

`cleanupServices` mostly duplicates `cleanup`’s logic (parse, filter, log). You can express the service special‑case via a `keep` predicate:

```ts
const cleanupServices = () => {
  const host = Cypress.env("SERVER_NAME") as string;
  const keepPrefixes = [
    `HTTP/${host}`,
    `DNS/${host}`,
    `dogtag/${host}`,
    `ipa-dnskeysyncd/${host}`,
    `ldap/${host}`,
  ];

  return cleanup("service", {
    keep: (u) => !keepPrefixes.some((prefix) => u.startsWith(prefix)),
  });
};
```

If you need to keep using `cy.exec("podman exec webui ipa service-find ...")` specifically for services, you could extend `cleanup` with an optional `execFind` callback, but you still benefit from sharing the parsing/logging logic:

```ts
type CleanupOptions = {
  omit?: string[];
  keep?: (value: string) => boolean;
  execFind?: () => Cypress.Chainable<Cypress.Exec>;
};

export const cleanup = (name: string, options: CleanupOptions = {}) => {
  const { omit = [], keep, execFind } = options;
  const findCmd = `${name}-find --pkey-only`;
  const deleteCmd = `${name}-del`;

  const runFind = execFind
    ? execFind
    : () => cy.ipa(findCmd, undefined, { failOnNonZeroExit: false });

  echoToTerminal(
    `cleanup starting: find=[${findCmd}] delete=[${deleteCmd}] omit=[${omit}]`
  );

  runFind().then((result) => {
    // same parsing/filtering body as above
  });
};

// services:
const cleanupServices = () => {
  const host = Cypress.env("SERVER_NAME") as string;
  const keepPrefixes = [/* ... */];

  return cleanup("service", {
    keep: (u) => !keepPrefixes.some((prefix) => u.startsWith(prefix)),
    execFind: () =>
      cy.exec(`podman exec webui ipa service-find --pkey-only`, {
        failOnNonZeroExit: false,
      }),
  });
};
```

### 3. Make the `Before` hook data‑driven

The current `Before` block is long and repetitive, which makes it harder to see what’s different for each resource. With the simplified `cleanup` API you can compress this into a config array:

```ts
const CLEANUPS: { name: string; omit?: string[] }[] = [
  { name: "automember" },
  { name: "certmaprule" },
  { name: "dnsforwardzone" },
  { name: "dnsrecord" },
  { name: "dnszone" },
  {
    name: "group",
    omit: ["admins", "editors", "ipausers", "trust admins"],
  },
  { name: "hbacrule" },
  { name: "hbacsvc" },
  { name: "hbacsvcgroup" },
  { name: "host" },
  { name: "hostgroup" },
  { name: "idoverridegroup" },
  { name: "idoverrideuser" },
  { name: "idp" },
  { name: "idrange" },
  { name: "idview" },
  { name: "netgroup" },
  { name: "pwpolicy" },
  { name: "stageuser" },
  { name: "sudocmd" },
  { name: "sudocmdgroup" },
  { name: "sudorule" },
  { name: "trust" },
  { name: "user", omit: ["admin"] },
];

Before(() => {
  cy.log("running cleanup");
  CLEANUPS.forEach(({ name, omit }) => cleanup(name, { omit }));
  cleanupServices(); // or fold into CLEANUPS if generalized fully
});
```

This preserves all existing behavior but makes the structure much easier to scan and extend.

### 4. Decouple and standardize logging

`echoToTerminal` hardcodes the log path and you manually build similar start/summary messages in multiple places. A tiny wrapper can centralize that and make the log path configurable:

```ts
const LOG_PATH =
  Cypress.env("CLEANUP_LOG_PATH") ??
  "/home/vmiticka/Programming/freeipa-webui/cypress/support/logs.log";

const echoToTerminal = (message: string) => {
  const escaped = message.replace(/"/g, '\\"');
  cy.exec(`echo "${escaped}" >> ${LOG_PATH}`);
};

const logCleanupStart = (name: string, details: string) =>
  echoToTerminal(`cleanup[${name}] start: ${details}`);

const logCleanupSummary = (name: string, total: number) =>
  echoToTerminal(`cleanup[${name}] done: total=${total}`);
```

Then `cleanup`/`cleanupServices` only call `logCleanupStart`/`logCleanupSummary`, which reduces duplication and keeps the log format consistent.
</issue_to_address>

---

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

• X
• Mastodon
• LinkedIn
• Facebook

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}

[duzda]

So apparently last time I was reviewing I forgot to press the Submit review button...

[duzda]

In any case, the code looks fine and should be reasonably fast, I will test this locally tomorrow.

[duzda]

I think you will have to run kinit and there's no way around that. You might get around it by running it just once. Here goes the command podman exec webui echo "Secret123" | podman exec -i webui kinit

[duzda]

Looking at my local, the whole deletion takes about 2 seconds, which is reasonably good, especially considering there is a handful of useless calls you can shed easily.

[veronnicka]

So I think the POC is ready for review. I still need to figure out some calls (see commented deletion calls) but i will figure this out in the PRs implementing the changes.

The cleanup uses a Before hook, this runs before each scenario. In this hook, a cy.task is run in order to paralelize the deletion calls. I also had to run kinit before both deletion or adding elements.

The seed adds a new cy.ipa calls which runs podman exec webui ipa via cy.exec(). The seed call has to be inside the scenarios, in the begining, otherwise the cleanup call would delete it.

[duzda]

Hi, a lot to go through. Also performance note, we're moving from 50 minutes to 2 hours 30 minutes, which sucks, a lot, it should be the other way around. Let's not focus on that right now, let's get the code clean and simpler, then we can shift to performance.

[duzda]

Please move this to the support folder.

[veronnicka]

About this, I cant find a way to put it in the support folder as apparently you cant import hooks into the support/e2e file.

[veronnicka]

Thank you for your feedback, I adjusted the code. Ad the slow down, it is the cleanup after each scenario thats slowing it down. Also, not all of the seeds are implemented yet so thats gonna speed it up a bit

[carma12]

Why does this command has the --force parameter? I don't see it used anywhere else.

[veronnicka]

I simply re-wrote the step, the original test was run with the force as well. I think here the force means it does not check if the host has a network or sth like that, which we dont really need for this test. But thats just from what I remember.

[rjeffman]

@veronnicka is correct. From the command --help: --force: force host name even if not in DNS, and we don't need/want a DNS record here.

[rjeffman]

I would not use this sentence for the Given, it doesn't made clear what "view" is. In my opinion `Given idview "a_new_view" exists" is more clear and will never be ambiguous.

I know it was originally like that, but this is a good time to fix these issues.

[rjeffman]

I'm not sure what this error means, but for all the commented out commands, in ansible-freeipa we needed to add --continue for it to work.

I assume I don't fully understand the issue and when --continue is required.

[veronnicka]

I left the commented out commands to investigate later, with the implementation of all the cleanup tests, if we like this solution. There were the ones that didnt work the "traditional" way and need to be investigated, either they need some special parameter or maybe they wont be needed at all.

[duzda]

Error // TODO: Check - "Could not get Grouping Type interactively" error means that the command is missing some arguments, those missing arguments are obtained interactively, of course in CI there's no stdin for the user to connect to, so it fails.

The fix is simple, just run this command locally, figure out what's missing, fill in the missing params.

[rjeffman]

If I understood correctly we will try to cleanup everything in the FreeIPA server (based on what is on CLEANUP_SPECS) before every scenario. As a find is performed for all supported objects it will add a good amount of load on the server for each scenario.

We either need to find a way to define which objects to be cleaned up for each scenario, or we should only clean up everything before each feature. As more objects are supported we'll be adding more overhead for each scenario.

[veronnicka]

"We either need to find a way to define which objects to be cleaned up for each scenario" - we were kind of doing this with the @cleanup approach before. A way to make it better would be to at least replace the @ cleanup with call directly to the terminal, same as the @seed .

We should only clean up everything before each feature - Yes, this could be done for sure. But will reduce the benefit of the cleanup call only for the begining, which is a pity.

Should I make the changes then? Or should we discuss this further.

[rjeffman]

I think we need to discuss it further. Integration tests is already taking ~2.5 hours to complete.

[duzda]

Let's not make guesses and measure, that this is truly the bottleneck.