Conversation
…r-cache Co-authored-by: nccatoni <222672590+nccatoni@users.noreply.github.com> # Conflicts: # utils/scripts/load-binary.sh
Co-authored-by: nccatoni <222672590+nccatoni@users.noreply.github.com>
|
I can only run on private repositories. |
|
|
|
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 55886cf0f8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if environment == "custom": | ||
| return |
There was a problem hiding this comment.
When a reused binaries/ directory was previously staged as dev or prod, returning here leaves its generated selector active during a subsequent custom run. This can silently select the wrong tracer: for example, with two custom Python wheels, utils/build/docker/python/install_ddtrace.sh skips the single-wheel branch, sees the stale python-load-from-s3 entry at lines 18–31, and never reaches the version-specific wheel selection at lines 38–42. Custom mode should remove entries owned by this target while preserving unowned payloads.
Useful? React with 👍 / 👎.
| if path.exists() and existing_owner is None: | ||
| raise TargetArtifactError(f"Refusing to overwrite unowned artifact entry '{filename}'") |
There was a problem hiding this comment.
When a user edits a previously generated selector, its manifest owner remains present, so this check permits staging to overwrite it; the stale-entry loop likewise deletes modified files. The recorded sha256 is never compared with the current content, meaning manual changes can be silently lost and experiments can run against a different artifact than intended. Before overwriting or deleting an owned entry, verify that its current hash still matches the manifest and refuse the operation otherwise.
Useful? React with 👍 / 👎.
|
|
||
|
|
||
| @scenarios.test_the_test | ||
| class Test_LoadBinaryPython: |
There was a problem hiding this comment.
The newly added Test_LoadBinaryPython, Test_TargetArtifactStaging, Test_TargetArtifactResolvers, and Test_TargetArtifactModules classes have scenario decorators but no feature decorator. Repository rules require every test class to use either a specific @features.* association or @features.not_reported, so these framework tests should be explicitly marked rather than omitted from feature metadata.
AGENTS.md reference: AGENTS.md:L9-L17
Useful? React with 👍 / 👎.
| path = binaries_dir / filename | ||
| path.parent.mkdir(parents=True, exist_ok=True) | ||
| path.write_text(entry.content, encoding="utf-8") |
There was a problem hiding this comment.
If an entry path is a broken symlink, path.exists() is false, so it is treated as safe and the later write_text() follows the link, creating or overwriting its target outside binaries/; an owned symlink can similarly overwrite an existing external file. This bypasses the filename traversal checks and can corrupt unrelated workspace files. Reject symlink entry paths or publish entries with an atomic replacement that replaces the link itself.
Useful? React with 👍 / 👎.
| module = importlib.util.module_from_spec(spec) | ||
| spec.loader.exec_module(module) |
There was a problem hiding this comment.
The module created here is executed without first being inserted into sys.modules. A target artifact.py that defines a normal @dataclass with postponed annotations then fails during import because dataclasses looks up cls.__module__ in sys.modules and receives None; other import-time tooling can rely on the same invariant. Register the synthetic module name before exec_module, restoring or removing it if execution fails.
Useful? React with 👍 / 👎.
|
|
||
| class Prod(SimpleTarget): | ||
| inputs = (PypiLatestResolver(name="ddtrace", package="ddtrace"),) | ||
| entries = (text_entry("python-load-from-pip", "ddtrace=={ddtrace.version}"),) |
There was a problem hiding this comment.
If binaries/ already contains an unowned python-load-from-s3 file from a legacy or manual development run, production staging adds this python-load-from-pip entry without detecting the conflict. The Python installer checks the S3 selector first at utils/build/docker/python/install_ddtrace.sh lines 18–31, so the new production selector is ignored and the tests silently exercise the development artifact. Production staging should refuse this conflicting manual selector rather than report success with an ineffective entry.
Useful? React with 👍 / 👎.
| resolved_inputs = { | ||
| artifact_resolver.name: artifact_resolver.resolve(env) | ||
| for artifact_resolver in target_environment.artifact_inputs(env) | ||
| } |
There was a problem hiding this comment.
When a target declares two inputs with the same name, this dictionary comprehension silently discards the first resolved value while still performing both external lookups. Templates then bind that name to the later artifact, which can stage a valid-looking but unintended selector when both resolvers return compatible models. Detect duplicate resolver names and fail before performing resolution.
Useful? React with 👍 / 👎.
2 participants
Motivation
Artifact selection is currently centralized in a large compatibility script and often relies on mutable selectors. A smaller, target-owned staging model is needed to make selected artifacts explicit, bounded, and safe to refresh before builds consume them.
This PR intentionally introduces the framework with Python as the only migrated language so the API and ownership model can be reviewed independently from the remaining language migrations and GitLab cache integration.
Changes
binaries/.Testing
TEST_THE_TESTfocused suite: 62 passed for staging, resolvers, manifest safety, compatibility loading, and Python dev/custom behavior.Workflow
🚀 Once your PR is reviewed and the CI green, you can merge it!
🛟 #apm-shared-testing 🛟
Reviewer checklist
tests/ormanifests/is modified ? I have the approval from R&P teambuild-XXX-imagelabel is presentPR by Bits - View session in Datadog
Comment @DataDog to request changes