Skip to content

Conversation

@owtaylor
Copy link
Collaborator

The initial set of testing farm requests that we supported reproducing were requests as created by NEWA with the build specified via the BUILDS environment variable, but you can also specify a build using the artifacts parameter to the testing farm API.

If we don't find the BUILDS parameter, look for an artifact in the test request and use that to specify the build to run the tests with.

Also add support for such requests to TestingFarmRequest.build_nvr

The initial set of testing farm requests that we supported reproducing
were requests as created by NEWA with the build specified via the
BUILDS environment variable, but you can also specify a build using
the artifacts parameter to the testing farm API.

If we don't find the BUILDS parameter, look for an artifact in the
test request and use that to specify the build to run the tests with.

Also add support for such requests to TestingFarmRequest.build_nvr
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request extends test reproduction to support builds specified as Testing Farm artifacts, in addition to the existing BUILDS environment variable. The changes in supervisor_types.py correctly expand build_nvr to find build IDs from artifacts. However, the implementation in supervisor/testing_farm_utils.py for creating the new test environment has a few critical issues. It's susceptible to KeyError exceptions, can lead to data loss by dropping existing artifacts, and may incorrectly replace unrelated artifacts. I've provided suggestions to make the logic safer and more robust.

Comment on lines +106 to +111
builds_var = env["variables"].get("BUILDS")
if builds_var is not None:
new_env["variables"] = env["variables"] | {"BUILDS": build_nvr}
return new_env

new_env["variables"] = env["variables"]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

critical

This block has two critical issues:

  1. KeyError risk: It accesses env["variables"] directly (lines 106, 108, 111), which will raise a KeyError and crash the process if an environment lacks a variables key.
  2. Data loss: If the BUILDS variable is found, the function returns immediately (line 109), causing any other artifacts present in the original environment to be dropped from the new one.

Here is a safer implementation that addresses both problems.

Suggested change
builds_var = env["variables"].get("BUILDS")
if builds_var is not None:
new_env["variables"] = env["variables"] | {"BUILDS": build_nvr}
return new_env
new_env["variables"] = env["variables"]
variables = env.get("variables", {})
if "BUILDS" in variables:
new_env["variables"] = variables | {"BUILDS": build_nvr}
if "artifacts" in env:
new_env["artifacts"] = env["artifacts"]
return new_env
new_env["variables"] = variables

Comment on lines +113 to +122
artifacts = env.get("artifacts")
if artifacts and len(artifacts) == 1:
new_env["artifacts"] = [
{
"id": build_nvr,
"type": "redhat-brew-build",
"order": 40,
}
]
return new_env
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

The logic to replace an artifact is not safe. It assumes that if an environment has exactly one artifact, it must be the build artifact. This can lead to replacing an unrelated artifact if it happens to be the only one. The code should verify that the artifact's type is redhat-brew-build before replacing it.

Additionally, the order for the new artifact is hardcoded to 40. It would be more robust to preserve the order from the original artifact if it exists.

        artifacts = env.get("artifacts")
        if (
            artifacts
            and len(artifacts) == 1
            and artifacts[0].get("type") == "redhat-brew-build"
        ):
            new_env["artifacts"] = [
                {
                    "id": build_nvr,
                    "type": "redhat-brew-build",
                    "order": artifacts[0].get("order", 40),
                }
            ]
            return new_env

Copy link
Collaborator

@Jazzcort Jazzcort left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants