Datadog-sync CLI#1
Merged
Merged
Conversation
zippolyte
reviewed
May 12, 2021
michael-richey
added a commit
that referenced
this pull request
Apr 27, 2026
Fixes two P2 bugs in --force-missing-dependencies import discovery: Bug #1 (destination state leak): _discover_missing_dependencies() was reusing _resource_connections() → connect_id(), which checks config.state.destination first. If a dep existed in stale destination state, it appeared "resolved" and was never checked against source, causing it to be silently skipped even when not imported. Bug #2 (no closure through present deps): Discovery only scanned resources_arg types. Deps already in source state from a prior partial import had their own transitive deps ignored. Example: dashboard_list → dashboard (present) → monitor (missing) — monitor was never found. Fix: Introduce a parallel import-time path: - BaseResource.extract_source_ids(): source-only mirror of connect_id, no destination state check, no mutation. Overridden in Monitors, RestrictionPolicies, TeamMemberships, SyntheticsTests for types that need regex/prefix/type-dispatch logic. - ResourcesHandler._dep_in_source_state(): exact + prefix match for composite synthetics_tests keys ('{public_id}#{monitor_id}'). - ResourcesHandler._source_dependencies_for_resource(): find_attr() traversal using extract_source_ids instead of connect_id. - Rewrite _discover_missing_dependencies() as a BFS closure walk that seeds from resources_arg nodes, follows edges through already-present source resources, and collects only nodes not yet in source state. - Update _import_missing_dep_cb() transitive discovery to use the same source-only path. The sync-time path (_resource_connections, connect_id, get_dependency_graph) is entirely untouched. Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
michael-richey
added a commit
that referenced
this pull request
Apr 28, 2026
Fixes two P2 bugs in --force-missing-dependencies import discovery: Bug #1 (destination state leak): _discover_missing_dependencies() was reusing _resource_connections() → connect_id(), which checks config.state.destination first. If a dep existed in stale destination state, it appeared "resolved" and was never checked against source, causing it to be silently skipped even when not imported. Bug #2 (no closure through present deps): Discovery only scanned resources_arg types. Deps already in source state from a prior partial import had their own transitive deps ignored. Example: dashboard_list → dashboard (present) → monitor (missing) — monitor was never found. Fix: Introduce a parallel import-time path: - BaseResource.extract_source_ids(): source-only mirror of connect_id, no destination state check, no mutation. Overridden in Monitors, RestrictionPolicies, TeamMemberships, SyntheticsTests for types that need regex/prefix/type-dispatch logic. - ResourcesHandler._dep_in_source_state(): exact + prefix match for composite synthetics_tests keys ('{public_id}#{monitor_id}'). - ResourcesHandler._source_dependencies_for_resource(): find_attr() traversal using extract_source_ids instead of connect_id. - Rewrite _discover_missing_dependencies() as a BFS closure walk that seeds from resources_arg nodes, follows edges through already-present source resources, and collects only nodes not yet in source state. - Update _import_missing_dep_cb() transitive discovery to use the same source-only path. The sync-time path (_resource_connections, connect_id, get_dependency_graph) is entirely untouched. Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
michael-richey
added a commit
that referenced
this pull request
Jun 12, 2026
* feat(sync): add --force-missing-dependencies support to import command Allows import to resolve and fetch all transitive dependencies, producing a self-contained local state so sync never calls the source API for missing deps in split import/sync workflows. Co-Authored-By: Claude Sonnet 4.6 <[email protected]> * fix(import): use source-only dep discovery with BFS closure walk Fixes two P2 bugs in --force-missing-dependencies import discovery: Bug #1 (destination state leak): _discover_missing_dependencies() was reusing _resource_connections() → connect_id(), which checks config.state.destination first. If a dep existed in stale destination state, it appeared "resolved" and was never checked against source, causing it to be silently skipped even when not imported. Bug #2 (no closure through present deps): Discovery only scanned resources_arg types. Deps already in source state from a prior partial import had their own transitive deps ignored. Example: dashboard_list → dashboard (present) → monitor (missing) — monitor was never found. Fix: Introduce a parallel import-time path: - BaseResource.extract_source_ids(): source-only mirror of connect_id, no destination state check, no mutation. Overridden in Monitors, RestrictionPolicies, TeamMemberships, SyntheticsTests for types that need regex/prefix/type-dispatch logic. - ResourcesHandler._dep_in_source_state(): exact + prefix match for composite synthetics_tests keys ('{public_id}#{monitor_id}'). - ResourcesHandler._source_dependencies_for_resource(): find_attr() traversal using extract_source_ids instead of connect_id. - Rewrite _discover_missing_dependencies() as a BFS closure walk that seeds from resources_arg nodes, follows edges through already-present source resources, and collects only nodes not yet in source state. - Update _import_missing_dep_cb() transitive discovery to use the same source-only path. The sync-time path (_resource_connections, connect_id, get_dependency_graph) is entirely untouched. Co-Authored-By: Claude Sonnet 4.6 <[email protected]> * fix(import): correct mobile app dep type + harden test fixture isolation P2: synthetics_tests referenceId was emitted under the wrong dep type. extract_source_ids (and connect_id) internally called super() with "synthetics_mobile_applications" but _source_dependencies_for_resource always tagged the result with the outer resource_connections dict key ("synthetics_mobile_applications_versions"). Import would then try to fetch the application ID from the versions endpoint. Fix: add "options.mobileApplication.referenceId" as a path under synthetics_mobile_applications in resource_connections, and split both connect_id and extract_source_ids into two explicit referenceId branches: - synthetics_mobile_applications + referenceType=latest → handle - synthetics_mobile_applications_versions + referenceType=latest → [] (and vice versa for pinned references). Each branch now produces the correct dep type without needing cross-type redirection. P3: import_test fixture (module-scoped config) did not save/restore destination state or force_missing_dependencies. Tests mutating either leaked state into later tests. Fix uses deepcopy to snapshot both source and destination state before each test and fully restores all mutated fields in the teardown. Co-Authored-By: Claude Sonnet 4.6 <[email protected]> * fix(sync): use canonical source key in BFS and fix SLO synthetics monitor ID discovery - Replace _dep_in_source_state (bool) with _source_state_key (Optional[str]) so BFS queues the actual dict key (e.g. 'pub#999') rather than the bare public_id, preventing KeyError in _source_dependencies_for_resource - Add SLO extract_source_ids override that mirrors connect_id's suffix-match logic, filtering out monitor_ids that are backed by synthetics_tests to prevent false ("monitors", id) missing-dep entries - Update enforcement test allowlist: remove service_level_objectives now that it has a custom extract_source_ids override - Rename 4 _dep_in_source_state tests to _source_state_key; add 2 regression tests covering the KeyError and SLO synthetics false-miss bugs Co-Authored-By: Claude Sonnet 4.6 <[email protected]> * fix(sync): lazy-load mobile app versions in SyntheticsTests.import_resource - Replace class-level mutable `versions: List = []` with instance-level `self.versions: Optional[List[Dict]] = None` in __init__ to prevent state leaking across instances - Add _ensure_mobile_versions_loaded(client) lazy loader that fetches SyntheticsMobileApplicationsVersions.get_resources() on first call only; uses None sentinel so orgs with zero versions don't refetch every import - Update get_resources() and import_resource() mobile branch to use the lazy loader, fixing the bug where force-missing-dep imports of mobile tests set mobileApplicationsVersions=[] because get_resources() was never called - Add 2 regression tests: lazy load on direct import, and None-vs-[] sentinel (no double fetch for empty version list) Co-Authored-By: Claude Sonnet 4.6 <[email protected]> * fix(import): unpack _sanitize_reason tuple in _import_missing_dep_cb _import_missing_dep_cb was passing reason=self._sanitize_reason(e) which passes the full (reason, failure_class) tuple as the reason kwarg. Unpack into _reason, _fc and pass both to _emit, matching the pattern used everywhere else _sanitize_reason is called. Update the 3 affected unit tests to assert the correct failure_class alongside reason: - SkipResource → failure_class="unknown" - CustomClientHTTPError(404) → failure_class="http_4xx_404" - RuntimeError → failure_class="unknown" Co-Authored-By: Claude Sonnet 4.6 <[email protected]> * style: apply black formatting to test_import_force_missing_deps.py Co-Authored-By: Claude Sonnet 4.6 <[email protected]> * fix(test): don't pass --force-missing-dependencies to import in integration helper The existing cassettes for test_resource_import were recorded without this flag. Passing it causes _import_missing_dep_cb to fire and make HTTP calls (e.g., fetching role/dashboard deps) that VCR blocks in record mode 'none', failing every test whose class sets force_missing_deps=True. The sync path already has cassettes recorded with the flag and is unaffected — leave that block in place. Co-Authored-By: Claude Sonnet 4.6 <[email protected]> * fix(test): remove stale test asserting --force-missing-dependencies on import test_integration_import_passes_force_missing_deps_flag asserted that import_resources() appends --force-missing-dependencies when force_missing_deps=True. That wiring was reverted (cassettes for test_resource_import were recorded without the flag, so VCR blocks the extra HTTP calls it triggers). Remove the test that asserts the reverted behaviour; keep test_integration_import_omits_flag_when_disabled as a guard that the flag never leaks back into the import path. Co-Authored-By: Claude Sonnet 4.6 <[email protected]> --------- Co-authored-by: Claude Sonnet 4.6 <[email protected]>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.