Skip to content

Datadog-sync CLI#1

Merged
skarimo merged 18 commits into
mainfrom
sherz/datadog-sync-cli-initial
May 13, 2021
Merged

Datadog-sync CLI#1
skarimo merged 18 commits into
mainfrom
sherz/datadog-sync-cli-initial

Conversation

@skarimo

@skarimo skarimo commented May 10, 2021

Copy link
Copy Markdown
Member

No description provided.

Comment thread Dockerfile Outdated
Comment thread Dockerfile Outdated
Comment thread README.md
Comment thread README.md
@skarimo
skarimo merged commit 06b74d2 into main May 13, 2021
@skarimo
skarimo deleted the sherz/datadog-sync-cli-initial branch May 13, 2021 17:55
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]>
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