[prabinovRedhat]

Summary

• Add optional storage_secret_extra in .providers.json copyoffload block for Forklift Secret stringData keys not covered by built-in vendor fields (e.g. PowerMax POWERMAX_PORT_GROUP_NAME, STORAGE_SKIP_SSL_VERIFICATION).
• Support COPYOFFLOAD_STORAGE_SECRET_EXTRA env var (JSON object) to override matching keys at runtime.
• Merge extras in the copy-offload storage secret fixture after base + vendor fields.
• Extend mtv-api-tests generate wizard with an optional prompt for extra keys.
• Document usage in the copy-offload guide, README, and .providers.json.example.

If storage_secret_extra is missing, empty, or null, behavior is unchanged.

Test plan

• TestCopyoffloadThinMigration on ocp-edge97 (ONTAP) — 6/6 passed (multiple runs)
• Verified storage Secret on cluster includes extra keys from storage_secret_extra (QE_TEST_EXTRA_KEY, ANOTHER_NON_VENDOR_KEY) plus STORAGE_* and ONTAP_SVM
• pre-commit run --all-files passed on changed files

Summary by CodeRabbit

• New Features

  • Allow configuring extra custom Kubernetes Secret keys for copy‑offload via provider config or the COPYOFFLOAD_STORAGE_SECRET_EXTRA environment variable; interactive wizard may prompt for these.

• Documentation

  • Added detailed guidance, examples, quoting/boolean rules, and precedence (env overrides provider) for supplying extra secret fields.

• Bug Fixes

  • Extra secret values are now masked in displayed configuration.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 2cb5c904-c9b6-43b9-801e-8ccd48f7e65f

📥 Commits

Reviewing files that changed from the base of the PR and between f48b1b3 and bce8be1.

📒 Files selected for processing (7)

• .providers.json.example
• README.md
• cli/mtv_api_tests/common.py
• cli/mtv_api_tests/generate.py
• guides/copyoffload/how-to-run-copyoffload-tests.md
• tests/copyoffload/conftest.py
• utilities/copyoffload_migration.py

---

Walkthrough

Adds support for extra Kubernetes Secret stringData entries for copy-offload via storage_secret_extra (provider config or COPYOFFLOAD_STORAGE_SECRET_EXTRA), with JSON parsing/normalization, env override precedence, interactive CLI collection, fixture merging, and updated docs/examples.

Changes

Storage Secret Extra Configuration

Layer / File(s)	Summary
Core storage secret extra parsing and merging utilities/copyoffload_migration.py	Adds json import and STORAGE_SECRET_EXTRA_ENV, parse_storage_secret_extra_env(), get_storage_secret_extra(), and merge_storage_secret_extra() to parse env JSON, normalize values (booleans->"true"/"false", trim, skip None), resolve precedence (provider config then env override), and merge extras into secret_data overriding duplicates.
Test fixture wiring and secret construction tests/copyoffload/conftest.py	Expands imports to include merge_storage_secret_extra, adds assert checks that storage credentials are not None, annotates secret_data as dict[str, str], and calls merge_storage_secret_extra() to inject extra fields into the final Secret payload.
Interactive CLI wizard gathering cli/mtv_api_tests/common.py, cli/mtv_api_tests/generate.py	Adds gather_storage_secret_extra() to interactively collect optional extra key/value pairs, integrates it into the mtv-api-tests generate copy-offload flow, and ensures mask_passwords() masks nested storage_secret_extra values. Only truthy results are included in generated config.
Documentation and configuration examples .providers.json.example, README.md, guides/copyoffload/how-to-run-copyoffload-tests.md	Adds a commented storage_secret_extra example in .providers.json.example, README guidance, and a detailed guide describing uppercase key conventions, boolean/quoting normalization, env var override (COPYOFFLOAD_STORAGE_SECRET_EXTRA), PowerMax example, and wizard prompting.

Sequence Diagram

sequenceDiagram
  participant User
  participant GenerateWizard as mtv-api-tests generate
  participant GatherFunc as gather_storage_secret_extra()
  participant Fixture as copyoffload_storage_secret fixture
  participant MergeFunc as merge_storage_secret_extra()
  participant Secret as Kubernetes Secret

  User->>GenerateWizard: run generate
  GenerateWizard->>GatherFunc: prompt for extra fields
  GatherFunc->>User: request key/value pairs
  User->>GatherFunc: provide extras or skip
  GatherFunc->>GenerateWizard: return storage_secret_extra dict
  GenerateWizard->>Fixture: build copyoffload config
  Fixture->>MergeFunc: merge_storage_secret_extra(secret_data, config)
  MergeFunc->>MergeFunc: parse env & resolve precedence
  MergeFunc->>Fixture: return updated secret_data
  Fixture->>Secret: create Secret with merged fields

Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Suggested labels

can-be-merged

Suggested reviewers

• myakove
• krcmarik
• solenoci

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and specifically describes the main change: adding a new storage_secret_extra feature for handling array-specific Kubernetes Secret keys in the copy-offload functionality.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[qodo-code-review]

Review Summary by Qodo

Add storage_secret_extra for array-specific Secret keys in copy-offload

✨ Enhancement

Walkthroughs

Description

• Add storage_secret_extra config option for array-specific Kubernetes Secret keys
• Support COPYOFFLOAD_STORAGE_SECRET_EXTRA environment variable override (JSON object)
• Extend mtv-api-tests generate wizard with interactive prompts for extra keys
• Merge extra secret entries after base and vendor-specific fields in storage secret fixture
• Document usage in copy-offload guide, README, and .providers.json.example

Diagram

flowchart LR
  A["providers.json<br/>storage_secret_extra"] -->|merge| C["Storage Secret<br/>stringData"]
  B["COPYOFFLOAD_STORAGE_SECRET_EXTRA<br/>env var"] -->|override| C
  D["Base + Vendor<br/>fields"] -->|base| C
  E["generate wizard<br/>prompts"] -->|populate| A

Loading

File Changes

1. cli/mtv_api_tests/common.py ✨ Enhancement +27/-0

Add interactive prompt for extra storage secret keys

• Add gather_storage_secret_extra() function to prompt for optional extra Secret key/value pairs
• Validates non-empty keys and values with interactive confirmation loop
• Provides guidance on using Forklift populator documentation for key names

cli/mtv_api_tests/common.py

---

2. cli/mtv_api_tests/generate.py ✨ Enhancement +4/-0

Integrate extra secret keys into generate wizard

• Import gather_storage_secret_extra from common module
• Call gather_storage_secret_extra() in _gather_copyoffload_config() after vendor fields
• Conditionally add storage_secret_extra to config if non-empty

cli/mtv_api_tests/generate.py

---

3. tests/copyoffload/conftest.py ✨ Enhancement +12/-2

Merge extra secret entries in storage secret fixture

• Import merge_storage_secret_extra from copyoffload_migration utilities
• Add type annotation for secret_data dictionary
• Add assertions to validate storage credentials are not None
• Call merge_storage_secret_extra() to merge extra entries after vendor fields

tests/copyoffload/conftest.py

---

View more (4)

4. utilities/copyoffload_migration.py ✨ Enhancement +120/-0

Add storage secret extra parsing and merging utilities

• Add STORAGE_SECRET_EXTRA_ENV constant for environment variable name
• Implement _storage_secret_extra_value_as_string() to normalize values (booleans to lowercase)
• Implement _secret_extra_entries_from_mapping() to validate and normalize key/value pairs
• Implement parse_storage_secret_extra_env() to parse JSON from environment variable
• Implement get_storage_secret_extra() to resolve extras from config and env (env overrides
 config)
• Implement merge_storage_secret_extra() to merge extras into secret data with logging

utilities/copyoffload_migration.py

---

5. .providers.json.example 📝 Documentation +8/-0

Document storage_secret_extra in providers JSON example

• Add commented example of storage_secret_extra configuration for PowerMax vendor
• Document that keys must match Forklift Secret stringData names (uppercase)
• Note that env var COPYOFFLOAD_STORAGE_SECRET_EXTRA overrides config values

.providers.json.example

---

6. README.md 📝 Documentation +3/-0

Add storage_secret_extra reference to README

• Add brief reference to storage_secret_extra for array-specific Secret keys
• Direct users to copy-offload testing guide for detailed usage
• Mention both .providers.json and environment variable options

README.md

---

7. guides/copyoffload/how-to-run-copyoffload-tests.md 📝 Documentation +31/-0

Document storage_secret_extra usage in copy-offload guide

• Add comprehensive "Extra storage secret keys" section explaining storage_secret_extra
• Document that keys must match Forklift Secret stringData names (uppercase)
• Explain value merging behavior and override precedence
• Provide PowerMax example with JSON syntax for both config and environment variable
• Note that JSON booleans are normalized to lowercase "true"/"false" for Secret stringData
• Mention interactive wizard support for key entry

guides/copyoffload/how-to-run-copyoffload-tests.md

---

ⓘ You are approaching your monthly quota for Qodo. Upgrade your plan

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (5) 📘 Rule violations (2)

1. get_storage_secret_extra() docstring lacks Raises 📘 Rule violation ⚙ Maintainability ⭐ New

Description

The new get_storage_secret_extra() can raise ValueError (and may propagate JSON parsing/type
errors), and the new merge_storage_secret_extra() can propagate those exceptions as well, but both
docstrings omit a Raises section. This violates the required standard docstring sections for new
functions and reduces maintainability for callers and fixture authors.

Code

utilities/copyoffload_migration.py[R121-147]

Evidence

PR Compliance ID 15 requires new functions to include standard docstring sections, including a
Raises section when exceptions are raised. The evidence notes that get_storage_secret_extra() is
newly added and contains an explicit raise ValueError(...) yet its docstring documents only
Args/Returns, and that merge_storage_secret_extra() is also newly added and calls
get_storage_secret_extra(), meaning it can propagate ValueError/TypeError (and other
parsing-related errors) without documenting them because its docstring likewise only includes
Args/Returns.

CLAUDE.md: New Functions Must Include Standard Docstring Sections (Args/Returns/Raises) As Applicable
utilities/copyoffload_migration.py[121-147]
utilities/copyoffload_migration.py[150-175]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Both `get_storage_secret_extra()` and `merge_storage_secret_extra()` can raise or propagate exceptions (including `ValueError` and other parsing/type errors) but their docstrings do not include a `Raises` section, violating the required standard docstring sections for new functions.

## Issue Context
- `get_storage_secret_extra()` raises `ValueError` when `storage_secret_extra` is not a mapping, and it may also propagate exceptions from `parse_storage_secret_extra_env()` and `_secret_extra_entries_from_mapping()` (e.g., JSON parsing/type errors).
- `merge_storage_secret_extra()` is new, participates in secret construction, and calls `get_storage_secret_extra()`, so it can propagate the same exceptions; documenting these failure modes is important for callers and fixture authors.

## Fix Focus Areas
- utilities/copyoffload_migration.py[121-147]
- utilities/copyoffload_migration.py[150-175]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. Wizard echoes secret values 🐞 Bug ⛨ Security ⭐ New

Description

gather_storage_secret_extra() reads extra Secret values with Prompt.ask() without password
masking, so values are visible on-screen while typing. This can leak credentials/tokens intended to
go into Kubernetes Secret stringData.

Code

cli/mtv_api_tests/common.py[R259-266]

Evidence

The new wizard function prompts the extra value without password=True, while the same CLI uses
masked prompts for other secret inputs (storage password).

cli/mtv_api_tests/common.py[245-269]
cli/mtv_api_tests/common.py[355-366]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`gather_storage_secret_extra()` collects values for Kubernetes Secret `stringData`, but it prompts the value in plaintext. This can expose sensitive values via shoulder-surfing or screen recording.

### Issue Context
Other credential prompts in this CLI already use masked input (`password=True`) for secret values.

### Fix Focus Areas
- cli/mtv_api_tests/common.py[245-269]

### Suggested fix
- Change the value prompt to use `Prompt.ask(..., password=True)` (or prompt for whether to mask, defaulting to masked).
- Keep the key prompt unmasked.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

3. Extras printed in summary 🐞 Bug ⛨ Security ⭐ New

Description

mtv-api-tests generate prints a configuration summary after collecting storage_secret_extra, but
mask_passwords() only masks keys containing "password"/"pwd". As a result, storage_secret_extra
values can be printed in plaintext even though they are destined for a Kubernetes Secret.

Code

cli/mtv_api_tests/generate.py[R157-163]

Evidence

generate.py prints a JSON summary after applying mask_passwords(), but mask_passwords() only
masks based on key names containing password/pwd, so arbitrary secret extra keys (e.g., tokens) will
remain visible.

cli/mtv_api_tests/generate.py[136-199]
cli/mtv_api_tests/generate.py[324-333]
cli/mtv_api_tests/common.py[767-784]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
The generate wizard prints a JSON "Configuration Summary" to stdout. With the new `storage_secret_extra` feature, arbitrary Secret `stringData` values may appear in that summary, but `mask_passwords()` does not mask them unless the *key name* contains "password"/"pwd".

### Issue Context
`storage_secret_extra` is explicitly for Kubernetes Secret `stringData` keys, so values should be treated as sensitive by default.

### Fix Focus Areas
- cli/mtv_api_tests/generate.py[136-200]
- cli/mtv_api_tests/generate.py[324-333]
- cli/mtv_api_tests/common.py[767-784]

### Suggested fix
- Update `mask_passwords()` to special-case `storage_secret_extra` (and mask all values under it as `"***"`).
 - Example behavior: if `key == "storage_secret_extra"` and `value` is a dict, return `{k: "***" for k in value}`.
- Alternatively (or additionally), omit `storage_secret_extra` from the summary output entirely.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

View more (1)

4. _storage_secret_extra_value_as_string missing docstring sections 📘 Rule violation ⚙ Maintainability

Description

The new helper _storage_secret_extra_value_as_string(value: Any) -> str has a docstring but does
not include the required Args and Returns sections. This reduces consistency and makes the
function harder to use/maintain compared to the project’s docstring standard.

Code

utilities/copyoffload_migration.py[R60-67]

Evidence

PR Compliance ID 17 requires new functions to use standard docstring sections, including Args when
parameters exist and Returns when returning a meaningful value. The newly added
_storage_secret_extra_value_as_string(value: Any) -> str includes neither Args nor Returns in
its docstring.

CLAUDE.md: All New Functions Must Use the Standard Docstring Sections (Args/Returns/Raises) as Applicable
utilities/copyoffload_migration.py[60-67]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
A newly added function `_storage_secret_extra_value_as_string(value: Any) -> str` has parameters and a non-None return, but its docstring does not include the standard docstring sections (`Args`, `Returns`) required by the project.

## Issue Context
This function was introduced as part of the `storage_secret_extra` support and should conform to the standard docstring format used across the codebase for consistency.

## Fix Focus Areas
- utilities/copyoffload_migration.py[60-67]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

5. Base secret keys overridable 🐞 Bug ≡ Correctness ⭐ New

Description

merge_storage_secret_extra() overwrites any existing secret_data key with values from
storage_secret_extra, allowing accidental replacement of required
STORAGE_HOSTNAME/STORAGE_USERNAME/STORAGE_PASSWORD. This can produce an invalid storage Secret
despite correct base credentials being supplied via the dedicated fields/env vars.

Code

utilities/copyoffload_migration.py[R166-174]

Evidence

The storage Secret is built with required base keys, then the new merge function blindly assigns
merged[secret_key] = value for every extra entry, which can overwrite those required keys.

tests/copyoffload/conftest.py[231-236]
utilities/copyoffload_migration.py[150-174]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`storage_secret_extra` is intended for *additional* Secret `stringData` keys (and optionally overriding vendor-mapped keys). The current merge function allows overriding required base credential keys too, which can silently break copy-offload configuration.

### Issue Context
Base keys are always required and already have dedicated config/env support (`storage_hostname`, `storage_username`, `storage_password`). Allowing them to be overridden via `storage_secret_extra` creates a footgun.

### Fix Focus Areas
- utilities/copyoffload_migration.py[150-174]
- tests/copyoffload/conftest.py[231-236]

### Suggested fix
- In `merge_storage_secret_extra()` (or in `get_storage_secret_extra()`), reject `storage_secret_extra` entries whose Secret key is in a reserved set:
 - `{"STORAGE_HOSTNAME", "STORAGE_USERNAME", "STORAGE_PASSWORD"}`
 - Raise `ValueError` with a clear message telling users to use the standard fields/env vars instead.
- (Optional) Adjust logging to indicate when an extra key overrides an existing non-reserved key (e.g., vendor-mapped fields).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

6. Extra values are stripped 🐞 Bug ☼ Reliability

Description

Extra Secret values are normalized with .strip() (and the wizard also strips), which can silently
change user-provided Secret stringData (notably if leading/trailing whitespace or newlines are
significant). This behavior is inconsistent with base copy-offload credentials, which are not
stripped.

Code

utilities/copyoffload_migration.py[R60-68]

Evidence

The new helper strips values, and the wizard strips values too; meanwhile the existing credential
path does not strip, demonstrating inconsistent and potentially lossy behavior.

utilities/copyoffload_migration.py[60-68]
cli/mtv_api_tests/common.py[258-267]
utilities/copyoffload_migration.py[33-58]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`storage_secret_extra` values are converted via `str(value).strip()` and the CLI wizard also applies `value.strip()`. This can silently modify Secret payloads and differs from the handling of other copy-offload credentials.

### Issue Context
- Base credentials returned by `get_copyoffload_credential()` are not stripped.
- Extra values are stripped in both env/config parsing and interactive generation.

### Fix Focus Areas
- Stop stripping in value conversion; perform emptiness checks without mutating the stored value:
 - `utilities/copyoffload_migration.py[60-68]`
 - `utilities/copyoffload_migration.py[70-93]`
- Align the wizard to the same semantics (validate non-empty, but avoid forcing `.strip()` into stored value unless explicitly desired/documented):
 - `cli/mtv_api_tests/common.py[245-269]`

### Suggested implementation notes
- Change `_storage_secret_extra_value_as_string` to:
 - return lowercase `"true"/"false"` for bools
 - for strings, return as-is (or at most preserve exact string while checking `if not value.strip(): ...`)
 - for non-strings, use `str(value)` without `.strip()`
- In `_secret_extra_entries_from_mapping`, decide emptiness using `.strip()` but store the unstripped string to avoid silent mutation.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

7. Unvalidated extra secret keys 🐞 Bug ≡ Correctness

Description

storage_secret_extra accepts arbitrary key names (only rejecting empty keys) even though the
repo’s own guide states keys must be uppercase Forklift Secret stringData names; this can produce
misconfigured Secrets that are hard to diagnose. The interactive generator also does not
normalize/validate casing, making it easy to generate a config that violates the documented
contract.

Code

utilities/copyoffload_migration.py[R70-93]

Evidence

The repo guide and example require uppercase Forklift Secret key names, but the implementation only
strips and checks emptiness, and the wizard stores keys as-entered. This mismatch is visible
directly in the cited docs and code.

guides/copyoffload/how-to-run-copyoffload-tests.md[220-227]
utilities/copyoffload_migration.py[70-93]
cli/mtv_api_tests/common.py[245-269]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`storage_secret_extra` keys are only checked for non-empty strings, but the guide explicitly requires uppercase Forklift Secret `stringData` names. Without validation/normalization, the generator and env/config parsing can accept invalid/mis-cased keys and silently proceed.

### Issue Context
- The guide states keys must be uppercase Forklift Secret `stringData` names.
- Current normalization only strips whitespace and checks for empty keys.

### Fix Focus Areas
- Add validation and/or normalization in the shared normalization helper so it applies to both `.providers.json` and `COPYOFFLOAD_STORAGE_SECRET_EXTRA`:
 - `utilities/copyoffload_migration.py[70-93]`
- Optionally normalize to uppercase (or error with a clear message) in the interactive wizard to prevent generating invalid configs:
 - `cli/mtv_api_tests/common.py[245-269]`

### Suggested implementation notes
- Enforce `key == key.upper()` and a safe character set (e.g., `re.fullmatch(r"[-._A-Z0-9]+", key)`), raising `ValueError` with a message that references `storage_secret_extra` and the expected format.
- Consider normalizing wizard input with `key = secret_key.strip().upper()` and still validating it.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

Previous review results

Review updated until commit c0e7462

Results up to commit c0e7462

🐞 Bugs (2) 📘 Rule violations (1) 📎 Requirement gaps (0)

1. _storage_secret_extra_value_as_string missing docstring sections 📘 Rule violation ⚙ Maintainability

Description

The new helper _storage_secret_extra_value_as_string(value: Any) -> str has a docstring but does
not include the required Args and Returns sections. This reduces consistency and makes the
function harder to use/maintain compared to the project’s docstring standard.

Code

utilities/copyoffload_migration.py[R60-67]

Evidence

PR Compliance ID 17 requires new functions to use standard docstring sections, including Args when
parameters exist and Returns when returning a meaningful value. The newly added
_storage_secret_extra_value_as_string(value: Any) -> str includes neither Args nor Returns in
its docstring.

CLAUDE.md: All New Functions Must Use the Standard Docstring Sections (Args/Returns/Raises) as Applicable
utilities/copyoffload_migration.py[60-67]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
A newly added function `_storage_secret_extra_value_as_string(value: Any) -> str` has parameters and a non-None return, but its docstring does not include the standard docstring sections (`Args`, `Returns`) required by the project.

## Issue Context
This function was introduced as part of the `storage_secret_extra` support and should conform to the standard docstring format used across the codebase for consistency.

## Fix Focus Areas
- utilities/copyoffload_migration.py[60-67]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. Unvalidated extra secret keys 🐞 Bug ≡ Correctness

Description

storage_secret_extra accepts arbitrary key names (only rejecting empty keys) even though the
repo’s own guide states keys must be uppercase Forklift Secret stringData names; this can produce
misconfigured Secrets that are hard to diagnose. The interactive generator also does not
normalize/validate casing, making it easy to generate a config that violates the documented
contract.

Code

utilities/copyoffload_migration.py[R70-93]

Evidence

The repo guide and example require uppercase Forklift Secret key names, but the implementation only
strips and checks emptiness, and the wizard stores keys as-entered. This mismatch is visible
directly in the cited docs and code.

guides/copyoffload/how-to-run-copyoffload-tests.md[220-227]
utilities/copyoffload_migration.py[70-93]
cli/mtv_api_tests/common.py[245-269]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`storage_secret_extra` keys are only checked for non-empty strings, but the guide explicitly requires uppercase Forklift Secret `stringData` names. Without validation/normalization, the generator and env/config parsing can accept invalid/mis-cased keys and silently proceed.

### Issue Context
- The guide states keys must be uppercase Forklift Secret `stringData` names.
- Current normalization only strips whitespace and checks for empty keys.

### Fix Focus Areas
- Add validation and/or normalization in the shared normalization helper so it applies to both `.providers.json` and `COPYOFFLOAD_STORAGE_SECRET_EXTRA`:
 - `utilities/copyoffload_migration.py[70-93]`
- Optionally normalize to uppercase (or error with a clear message) in the interactive wizard to prevent generating invalid configs:
 - `cli/mtv_api_tests/common.py[245-269]`

### Suggested implementation notes
- Enforce `key == key.upper()` and a safe character set (e.g., `re.fullmatch(r"[-._A-Z0-9]+", key)`), raising `ValueError` with a message that references `storage_secret_extra` and the expected format.
- Consider normalizing wizard input with `key = secret_key.strip().upper()` and still validating it.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

3. Extra values are stripped 🐞 Bug ☼ Reliability

Description

Extra Secret values are normalized with .strip() (and the wizard also strips), which can silently
change user-provided Secret stringData (notably if leading/trailing whitespace or newlines are
significant). This behavior is inconsistent with base copy-offload credentials, which are not
stripped.

Code

utilities/copyoffload_migration.py[R60-68]

Evidence

The new helper strips values, and the wizard strips values too; meanwhile the existing credential
path does not strip, demonstrating inconsistent and potentially lossy behavior.

utilities/copyoffload_migration.py[60-68]
cli/mtv_api_tests/common.py[258-267]
utilities/copyoffload_migration.py[33-58]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`storage_secret_extra` values are converted via `str(value).strip()` and the CLI wizard also applies `value.strip()`. This can silently modify Secret payloads and differs from the handling of other copy-offload credentials.

### Issue Context
- Base credentials returned by `get_copyoffload_credential()` are not stripped.
- Extra values are stripped in both env/config parsing and interactive generation.

### Fix Focus Areas
- Stop stripping in value conversion; perform emptiness checks without mutating the stored value:
 - `utilities/copyoffload_migration.py[60-68]`
 - `utilities/copyoffload_migration.py[70-93]`
- Align the wizard to the same semantics (validate non-empty, but avoid forcing `.strip()` into stored value unless explicitly desired/documented):
 - `cli/mtv_api_tests/common.py[245-269]`

### Suggested implementation notes
- Change `_storage_secret_extra_value_as_string` to:
 - return lowercase `"true"/"false"` for bools
 - for strings, return as-is (or at most preserve exact string while checking `if not value.strip(): ...`)
 - for non-strings, use `str(value)` without `.strip()`
- In `_secret_extra_entries_from_mapping`, decide emptiness using `.strip()` but store the unstripped string to avoid silent mutation.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

ⓘ You are approaching your monthly quota for Qodo. Upgrade your plan

[redhat-qe-bot]

Report bugs in Issues

Welcome! 🎉

This pull request will be automatically processed with the following features:

🔄 Automatic Actions

• Reviewer Assignment: Reviewers are automatically assigned based on the OWNERS file in the repository root
• Size Labeling: PR size labels (XS, S, M, L, XL, XXL) are automatically applied based on changes
• Issue Creation: Disabled for this repository
• Branch Labeling: Branch-specific labels are applied to track the target branch
• Auto-verification: Auto-verified users have their PRs automatically marked as verified
• Labels: All label categories are enabled (default configuration)

📋 Available Commands

PR Status Management

• /wip - Mark PR as work in progress (adds WIP: prefix to title)
• /wip cancel - Remove work in progress status
• /hold - Block PR merging (approvers only)
• /hold cancel - Unblock PR merging
• /verified - Mark PR as verified
• /verified cancel - Remove verification status
• /reprocess - Trigger complete PR workflow reprocessing (useful if webhook failed or configuration changed)
• /regenerate-welcome - Regenerate this welcome message

Review & Approval

• /lgtm - Approve changes (looks good to me)
• /approve - Approve PR (approvers only)
• /automerge - Enable automatic merging when all requirements are met (maintainers and approvers only)
• /assign-reviewers - Assign reviewers based on OWNERS file
• /assign-reviewer @username - Assign specific reviewer
• /check-can-merge - Check if PR meets merge requirements

Testing & Validation

• /retest tox - Run Python test suite with tox
• /retest build-container - Rebuild and test container image
• /retest conventional-title - Validate commit message format
• /retest all - Run all available tests

Container Operations

• /build-and-push-container - Build and push container image (tagged with PR number)
  • Supports additional build arguments: /build-and-push-container --build-arg KEY=value

Cherry-pick Operations

• /cherry-pick <branch> - Schedule cherry-pick to target branch when PR is merged
  • Multiple branches: /cherry-pick branch1 branch2 branch3

Label Management

• /<label-name> - Add a label to the PR
• /<label-name> cancel - Remove a label from the PR

✅ Merge Requirements

This PR will be automatically approved when the following conditions are met:

1. Approval: /approve from at least one approver
2. Status Checks: All required status checks must pass
3. No Blockers: No wip, hold, has-conflicts labels and PR must be mergeable (no conflicts)
4. Verified: PR must be marked as verified

📊 Review Process

Approvers and Reviewers

Approvers:

• myakove
• solenoci

Reviewers:

• krcmarik
• myakove
• solenoci

Available Labels

• hold
• verified
• wip
• lgtm
• approve
• automerge

AI Features

• Conventional Title: Mode: fix (claude/claude-opus-4-6[1m])
• Cherry-Pick Conflict Resolution: Enabled (claude/claude-opus-4-6[1m])
• Test Oracle: Triggers: approved (cursor/gpt-5.4-xhigh-fast); /test-oracle can be used anytime

💡 Tips

• WIP Status: Use /wip when your PR is not ready for review
• Verification: The verified label is removed on new commits unless the push is detected as a clean rebase
• Cherry-picking: Cherry-pick labels are processed when the PR is merged
• Container Builds: Container images are automatically tagged with the PR number
• Permission Levels: Some commands require approver permissions
• Auto-verified Users: Certain users have automatic verification and merge privileges

For more information, please refer to the project documentation or contact the maintainers.

[prabinovRedhat]

cc: @tshefi @rgolangh

[qodo-code-review]

Code review by qodo was updated up to the latest commit c0e7462

[coderabbitai]

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@utilities/copyoffload_migration.py`:
- Around line 142-143: The type-check for storage_secret_extra currently raises
ValueError when config_extra is not a dict; change this to raise TypeError
instead. Locate the conditional that checks "if not isinstance(config_extra,
dict)" (in the function handling storage_secret_extra/config_extra) and replace
the raised exception from ValueError(...) to TypeError(...) while keeping the
existing error message text so callers receive a TypeError for incorrect types.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 204b675a-3d80-4036-ae8d-7135afae95b2

📥 Commits

Reviewing files that changed from the base of the PR and between 98e1e12 and c0e7462.

📒 Files selected for processing (7)

• .providers.json.example
• README.md
• cli/mtv_api_tests/common.py
• cli/mtv_api_tests/generate.py
• guides/copyoffload/how-to-run-copyoffload-tests.md
• tests/copyoffload/conftest.py
• utilities/copyoffload_migration.py

[coderabbitai]

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@cli/mtv_api_tests/common.py`:
- Around line 778-779: The masking branch for storage_secret_extra uses a dict
comprehension; replace it with dict.fromkeys to be more idiomatic: in the block
where key == "storage_secret_extra" and isinstance(value, dict) (which currently
assigns masked[key] = {secret_key: "***" for secret_key in value}), set
masked[key] = dict.fromkeys(value, "***") so keys remain the same and all values
are "***".

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 697e5969-ec62-4fd6-8880-31193ee2a5cb

📥 Commits

Reviewing files that changed from the base of the PR and between c0e7462 and f48b1b3.

📒 Files selected for processing (7)

• .providers.json.example
• README.md
• cli/mtv_api_tests/common.py
• cli/mtv_api_tests/generate.py
• guides/copyoffload/how-to-run-copyoffload-tests.md
• tests/copyoffload/conftest.py
• utilities/copyoffload_migration.py

[tshefi]

/lgtm

[redhat-qe-bot2]

Clean rebase detected — no code changes compared to previous head (f48b1b3).
The following labels were preserved: commented-prabinovRedhat, commented-coderabbitai[bot], lgtm-coderabbitai[bot], lgtm-tshefi.

[rgolangh]

/lgtm

[prabinovRedhat]

/verified

[myakove]

/lgtm
/approve

[redhat-qe-bot1]

New container for ghcr.io/redhatqe/mtv-api-tests:latest published

[prabinovRedhat]

/cherry-pick v2.11 v2.10

[redhat-qe-bot2]

Cherry-pick conflicts were resolved by AI

Cherry-picked PR feat(copyoffload): add storage_secret_extra for array-specific Secret keys into v2.10: #539
Conflicts were automatically resolved by AI (claude/claude-opus-4-6[1m]).

Manual verification is required — please review the changes and test before merging.

[redhat-qe-bot2]

Cherry-pick conflicts were resolved by AI

Cherry-picked PR feat(copyoffload): add storage_secret_extra for array-specific Secret keys into v2.11: #540
Conflicts were automatically resolved by AI (claude/claude-opus-4-6[1m]).

Manual verification is required — please review the changes and test before merging.