Skip to content

feat(connector): add on-prem Active Directory (LDAP) HR connector - #1900

Merged
cyberdima merged 6 commits into
mainfrom
feat/ad-hr-connector
Jul 28, 2026
Merged

feat(connector): add on-prem Active Directory (LDAP) HR connector#1900
cyberdima merged 6 commits into
mainfrom
feat/ad-hr-connector

Conversation

@cyberdima

@cyberdima cyberdima commented Jul 24, 2026

Copy link
Copy Markdown
Contributor

CDK Python connector querying AD over LDAP/LDAPS (ldap3). Sibling of ms-entra — same class_people/identity_inputs Silver contract, different transport. sAMAccountName → sam_account identity signal reconciles on-prem AD ↔ cloud Entra ↔ self-hosted Git.

  • source_active_directory/: AbstractSource, ldap_client, users stream (paged search, objectGUID-keyed, privacy allowlist)
  • dbt/: bronze_promoted, snapshot, fields_history, identity_inputs, to_class_people (mirror of ms_entra__* models)
  • docs/domain/connector/HR_IDENTITY_CONNECTOR_GUIDE.md: agent-facing recipe for HR/identity connector authoring (CDK-for-non-HTTP pattern)
  • secrets/connectors/active-directory.yaml.example: K8s Secret template

Summary by CodeRabbit

  • New Features
    • Added an on-premises Active Directory connector with LDAP/LDAPS user synchronization.
    • Produces unified people profiles plus identity signals, including account status and manager relationships.
    • Adds field history and more complete manager updates (including reassignment and removal handling) for downstream identity processing.
  • Documentation
    • Added an agent-facing connector authoring guide for HR/identity “Silver contract” connectors, including CDK recipe and checklist.
    • Expanded Active Directory connector docs and added a Kubernetes Secret example.

@cyberdima
cyberdima requested a review from a team as a code owner July 24, 2026 21:23
@coderabbitai

coderabbitai Bot commented Jul 24, 2026

Copy link
Copy Markdown

Review Change Stack

Warning

Review limit reached

@cyberantonz, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 48 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 279354df-0655-49ab-9db4-bfaf8d2657b9

📥 Commits

Reviewing files that changed from the base of the PR and between 151bdcc and 57157d7.

📒 Files selected for processing (2)
  • src/ingestion/connectors/hr-directory/active-directory/README.md
  • src/ingestion/connectors/hr-directory/active-directory/dbt/active_directory__manager_identity_inputs.sql
📝 Walkthrough

Walkthrough

Adds an Airbyte LDAP/LDAPS Active Directory connector with paged Bronze user extraction, normalized identity fields, SCD2 history, class_people mapping, manager relationships, identity inputs, deployment configuration, and authoring documentation.

Changes

Active Directory HR Connector

Layer / File(s) Summary
Connector runtime and configuration
docs/domain/connector/..., src/ingestion/connectors/hr-directory/active-directory/..., src/ingestion/secrets/...
Adds the Airbyte source, configuration schema, packaging, Docker image, deployment descriptor, Kubernetes Secret example, and connector documentation.
LDAP transport and Bronze users stream
src/ingestion/connectors/hr-directory/active-directory/source_active_directory/...
Connects to LDAP/LDAPS, normalizes GUIDs, timestamps, and account status, then emits paged Active Directory users with a Bronze JSON schema.
Silver snapshots and identity mappings
src/ingestion/connectors/hr-directory/active-directory/dbt/..., src/ingestion/silver/_shared/...
Adds Bronze promotion, snapshots, field history, class_people mapping, identity and manager inputs, schema tests, and shared dbt dependency references.

Estimated code review effort: 4 (Complex) | ~45 minutes

Possibly related PRs

Suggested reviewers: mitasovr

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 66.67% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly summarizes the main change: adding an on-prem Active Directory LDAP HR connector.
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.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/ad-hr-connector

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

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 10

🤖 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 `@docs/domain/connector/HR_IDENTITY_CONNECTOR_GUIDE.md`:
- Line 30: Add the `text` language identifier to both fenced code blocks in the
HR identity connector guide, including the blocks around the referenced
locations, so the diagrams satisfy markdownlint MD040.

In
`@src/ingestion/connectors/hr-directory/active-directory/dbt/active_directory__manager_identity_inputs.sql`:
- Around line 41-43: Update the manager change filtering in the active-directory
manager identity inputs query so empty managerDn changes are retained as DELETE
events rather than excluded. Emit DELETE rows for both parent_email and
parent_id relationship value types when managerDn becomes empty, while
preserving existing behavior for non-empty manager updates.
- Around line 38-41: Update the manager identity input query around the history
CTE and manager join to include changes to a manager’s mail or
userPrincipalName, not only managerDn changes. Fan those email-history events
out to every user whose current managerDn references the changed manager, while
preserving the existing parent_email output and direct-report mapping.

In
`@src/ingestion/connectors/hr-directory/active-directory/dbt/active_directory__to_class_people.sql`:
- Around line 37-40: Update the manager mapping around the manager_person_id
projection to leave manager_person_id null instead of selecting mgr.id, since
mgr.id is an AD source identifier. Preserve the existing parent_id and
parent_email identity signals so manager relationships are resolved through
identity resolution.

In
`@src/ingestion/connectors/hr-directory/active-directory/dbt/active_directory__users_snapshot.sql`:
- Around line 12-16: Update the snapshot’s check_cols configuration to include
managerDn alongside the other tracked user fields, ensuring manager-only changes
create a new version for active_directory__users_fields_history and reach
active_directory__manager_identity_inputs.

In `@src/ingestion/connectors/hr-directory/active-directory/pyproject.toml`:
- Around line 1-3: Update the build configuration in pyproject.toml to add the
required cf-studio-path setting with the value .cf-studio, preserving the
existing build-system configuration.

In
`@src/ingestion/connectors/hr-directory/active-directory/source_active_directory/ldap_client.py`:
- Around line 64-65: Update the configuration parsing for ad_use_ssl before
constructing Tls or the LDAP server so string values "true" and "false" are
converted to booleans, while invalid values are rejected. Preserve the default
of true when the setting is absent, and ensure the normalized value controls
LDAPS selection and port 636 behavior.
- Line 65: Update the TLS configuration in the LDAP client’s SSL setup to
require certificate and hostname validation instead of using Tls(validate=0).
Configure the trusted CA bundle through the existing connector configuration or
mounted certificate path so private AD certificates are supported without
falling back to CERT_NONE.

In
`@src/ingestion/connectors/hr-directory/active-directory/source_active_directory/source.py`:
- Around line 50-56: The check_connection() validation must reject empty
normalized insight_source_id and insight_tenant_id values before records are
emitted. In source.py, retrieve and trim both identity fields and return the
existing failure response when either is empty; in spec.json, add minLength: 1
to both identity field definitions so configuration validation enforces the same
requirement.
- Around line 73-91: Update the connection probe in check_connection to avoid
treating expected LDAP size limits as failures: use BASE search scope with an
(objectClass=*) filter, or explicitly handle LDAPSizeLimitExceededResult as
success. Preserve failure handling for other LDAP errors and unreadable search
bases.
🪄 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: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: f8f791fe-5560-49c3-b25b-e935068c3750

📥 Commits

Reviewing files that changed from the base of the PR and between 48f410e and 4602f94.

📒 Files selected for processing (21)
  • docs/domain/connector/HR_IDENTITY_CONNECTOR_GUIDE.md
  • src/ingestion/connectors/hr-directory/active-directory/Dockerfile
  • src/ingestion/connectors/hr-directory/active-directory/README.md
  • src/ingestion/connectors/hr-directory/active-directory/dbt/active_directory__bronze_promoted.sql
  • src/ingestion/connectors/hr-directory/active-directory/dbt/active_directory__identity_inputs.sql
  • src/ingestion/connectors/hr-directory/active-directory/dbt/active_directory__manager_identity_inputs.sql
  • src/ingestion/connectors/hr-directory/active-directory/dbt/active_directory__to_class_people.sql
  • src/ingestion/connectors/hr-directory/active-directory/dbt/active_directory__users_fields_history.sql
  • src/ingestion/connectors/hr-directory/active-directory/dbt/active_directory__users_snapshot.sql
  • src/ingestion/connectors/hr-directory/active-directory/dbt/schema.yml
  • src/ingestion/connectors/hr-directory/active-directory/descriptor.yaml
  • src/ingestion/connectors/hr-directory/active-directory/pyproject.toml
  • src/ingestion/connectors/hr-directory/active-directory/source_active_directory/__init__.py
  • src/ingestion/connectors/hr-directory/active-directory/source_active_directory/ldap_client.py
  • src/ingestion/connectors/hr-directory/active-directory/source_active_directory/source.py
  • src/ingestion/connectors/hr-directory/active-directory/source_active_directory/spec.json
  • src/ingestion/connectors/hr-directory/active-directory/source_active_directory/streams/__init__.py
  • src/ingestion/connectors/hr-directory/active-directory/source_active_directory/streams/users.py
  • src/ingestion/secrets/connectors/active-directory.yaml.example
  • src/ingestion/silver/_shared/class_people.sql
  • src/ingestion/silver/_shared/identity_inputs.sql

Comment thread docs/domain/connector/HR_IDENTITY_CONNECTOR_GUIDE.md Outdated
cyberdima added a commit that referenced this pull request Jul 25, 2026
Fixes 10 issues flagged in PR #1900: manager email/removal not
propagated to identity_inputs, manager_person_id wrongly set to an
AD source id, managerDn untracked in the snapshot, ad_use_ssl string
"false" always truthy, LDAPS skipping cert validation (CERT_NONE),
empty tenant/source id accepted, check_connection misreading an
expected sizeLimitExceeded as an unreadable search base, missing
cf-studio-path, and MD040 lint on the guide doc.

Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>

Signed-off-by: Dmitry.Saukh <cyberdima@gmail.com>
@cyberdima
cyberdima force-pushed the feat/ad-hr-connector branch from e596e60 to f867ae2 Compare July 25, 2026 00:42
@github-actions

Copy link
Copy Markdown
Contributor

⚠️ Regenerate the connectors-ddl snapshot

This PR changes src/ingestion/**. If your change affects any
bronze / silver / gold schema, regenerate the committed DDL snapshot
and include it in this PR:

cd src/ingestion/scripts/bootstrap-db
set -a; source pins.env; source .env; set +a
./bootstrap-db.sh connectors-config.yaml   # fresh ClickHouse 25.7.5
./dump-ddl.sh                              # writes scripts/connectors-ddl/*.sql

Commit the resulting scripts/connectors-ddl/*.sql diff. If nothing
changed, no snapshot update is needed. (Regeneration is manual for now.)

CDK Python connector querying AD over LDAP/LDAPS (ldap3).
Sibling of ms-entra — same class_people/identity_inputs Silver
contract, different transport. sAMAccountName → sam_account identity
signal reconciles on-prem AD ↔ cloud Entra ↔ self-hosted Git.

- source_active_directory/: AbstractSource, ldap_client, users stream
  (paged search, objectGUID-keyed, privacy allowlist)
- dbt/: bronze_promoted, snapshot, fields_history, identity_inputs,
  to_class_people (mirror of ms_entra__* models)
- docs/domain/connector/HR_IDENTITY_CONNECTOR_GUIDE.md: agent-facing
  recipe for HR/identity connector authoring (CDK-for-non-HTTP pattern)
- secrets/connectors/active-directory.yaml.example: K8s Secret template

Signed-off-by: Dmitry.Saukh <cyberdima@gmail.com>

# Conflicts:
#	src/ingestion/silver/_shared/identity_inputs.sql

Signed-off-by: Dmitry Saukh <38005371+cyberdima@users.noreply.github.com>
Fixes 10 issues flagged in PR #1900: manager email/removal not
propagated to identity_inputs, manager_person_id wrongly set to an
AD source id, managerDn untracked in the snapshot, ad_use_ssl string
"false" always truthy, LDAPS skipping cert validation (CERT_NONE),
empty tenant/source id accepted, check_connection misreading an
expected sizeLimitExceeded as an unreadable search base, missing
cf-studio-path, and MD040 lint on the guide doc.

Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>

Signed-off-by: Dmitry.Saukh <cyberdima@gmail.com>

Signed-off-by: Dmitry Saukh <38005371+cyberdima@users.noreply.github.com>
@cyberdima
cyberdima force-pushed the feat/ad-hr-connector branch from 15d1eab to bf34e99 Compare July 27, 2026 09:39
@cyberdima
cyberdima enabled auto-merge July 27, 2026 09:51

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 4

🧹 Nitpick comments (2)
src/ingestion/connectors/hr-directory/active-directory/source_active_directory/streams/users.py (1)

163-195: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low value

Consider requiring tenant_id/source_id in the schema, matching the stated framework-field contract.

The docstring at line 115-117 in source.py's CDK counterpart states tenant_id/source_id/unique_key are all mandatory framework fields, but only unique_key and id are in required here. Since these are always populated in _to_record, this is non-blocking, but adding them closes a latent gap if the record-building logic ever changes.

♻️ Proposed fix
-            "required": ["unique_key", "id"],
+            "required": ["unique_key", "id", "tenant_id", "source_id"],
🤖 Prompt for 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.

In
`@src/ingestion/connectors/hr-directory/active-directory/source_active_directory/streams/users.py`
around lines 163 - 195, Update the get_json_schema method’s required list to
include tenant_id and source_id alongside unique_key and id, matching the
framework-field contract and the fields populated by _to_record.
src/ingestion/connectors/hr-directory/active-directory/source_active_directory/source.py (1)

101-106: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Silent except Exception: pass on connection cleanup in both files. Both files swallow all unbind exceptions with no logging; the # noqa: BLE001 doesn't suppress Ruff's S110 (try-except-pass) hint, so this still surfaces in lint.

  • src/ingestion/connectors/hr-directory/active-directory/source_active_directory/source.py#L101-L106: log the unbind exception at debug level (or extend the noqa to cover S110) instead of silently passing.
  • src/ingestion/connectors/hr-directory/active-directory/source_active_directory/streams/users.py#L157-L161: apply the same debug-logging fix to the conn.unbind() cleanup in read_records.
♻️ Proposed fix (apply to both sites)
-                try:
-                    conn.unbind()
-                except Exception:  # noqa: BLE001 — best-effort cleanup
-                    pass
+                try:
+                    conn.unbind()
+                except Exception:  # noqa: BLE001 — best-effort cleanup
+                    logger.debug("LDAP unbind failed", exc_info=True)
🤖 Prompt for 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.

In
`@src/ingestion/connectors/hr-directory/active-directory/source_active_directory/source.py`
around lines 101 - 106, The conn.unbind() cleanup handlers in source.py lines
101-106 and users.py lines 157-161 silently swallow exceptions and trigger S110.
Update both cleanup blocks to log unbind failures at debug level while
preserving best-effort cleanup and exception suppression; use the surrounding
source flow and read_records method as anchors.

Source: Linters/SAST tools

🤖 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
`@src/ingestion/connectors/hr-directory/active-directory/dbt/active_directory__manager_identity_inputs.sql`:
- Around line 71-84: Deduplicate the rows produced by manager_email_changes
before they are emitted, grouping or applying distinctness by report,
updated_at, and resolved manager_email so simultaneous mail and
userPrincipalName history events yield one event. Preserve the existing coalesce
resolution and joins, while retaining separate rows when the report, timestamp,
or resolved email differs.
- Around line 71-84: Update manager_email_changes and the downstream
event-generation logic so a manager with both mail and userPrincipalName empty
produces a DELETE event for each affected direct report. Ensure the DELETE
clears the existing parent_email instead of being removed by the final non-empty
filter, while preserving current behavior for non-empty manager emails.
- Around line 52-53: Update all three manager joins in the active-directory
manager identity inputs query, including the joins around current_users and the
referenced location, to require matching tenant_id and source_id in addition to
the existing manager identifier predicate. Preserve the current join behavior
within the same tenant and source while preventing cross-tenant or cross-source
matches.

In `@src/ingestion/connectors/hr-directory/active-directory/README.md`:
- Around line 90-93: Update the privacy statement in the README to remove the
inaccurate “or other PII” claim. Describe that the explicit allowlist still
collects required identity fields, while excluding non-required fields such as
photos, postal addresses, and phone numbers.

---

Nitpick comments:
In
`@src/ingestion/connectors/hr-directory/active-directory/source_active_directory/source.py`:
- Around line 101-106: The conn.unbind() cleanup handlers in source.py lines
101-106 and users.py lines 157-161 silently swallow exceptions and trigger S110.
Update both cleanup blocks to log unbind failures at debug level while
preserving best-effort cleanup and exception suppression; use the surrounding
source flow and read_records method as anchors.

In
`@src/ingestion/connectors/hr-directory/active-directory/source_active_directory/streams/users.py`:
- Around line 163-195: Update the get_json_schema method’s required list to
include tenant_id and source_id alongside unique_key and id, matching the
framework-field contract and the fields populated by _to_record.
🪄 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: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: b1e64a49-5c4b-46db-9c91-1d7b7843834a

📥 Commits

Reviewing files that changed from the base of the PR and between 4602f94 and 151bdcc.

📒 Files selected for processing (21)
  • docs/domain/connector/HR_IDENTITY_CONNECTOR_GUIDE.md
  • src/ingestion/connectors/hr-directory/active-directory/Dockerfile
  • src/ingestion/connectors/hr-directory/active-directory/README.md
  • src/ingestion/connectors/hr-directory/active-directory/dbt/active_directory__bronze_promoted.sql
  • src/ingestion/connectors/hr-directory/active-directory/dbt/active_directory__identity_inputs.sql
  • src/ingestion/connectors/hr-directory/active-directory/dbt/active_directory__manager_identity_inputs.sql
  • src/ingestion/connectors/hr-directory/active-directory/dbt/active_directory__to_class_people.sql
  • src/ingestion/connectors/hr-directory/active-directory/dbt/active_directory__users_fields_history.sql
  • src/ingestion/connectors/hr-directory/active-directory/dbt/active_directory__users_snapshot.sql
  • src/ingestion/connectors/hr-directory/active-directory/dbt/schema.yml
  • src/ingestion/connectors/hr-directory/active-directory/descriptor.yaml
  • src/ingestion/connectors/hr-directory/active-directory/pyproject.toml
  • src/ingestion/connectors/hr-directory/active-directory/source_active_directory/__init__.py
  • src/ingestion/connectors/hr-directory/active-directory/source_active_directory/ldap_client.py
  • src/ingestion/connectors/hr-directory/active-directory/source_active_directory/source.py
  • src/ingestion/connectors/hr-directory/active-directory/source_active_directory/spec.json
  • src/ingestion/connectors/hr-directory/active-directory/source_active_directory/streams/__init__.py
  • src/ingestion/connectors/hr-directory/active-directory/source_active_directory/streams/users.py
  • src/ingestion/secrets/connectors/active-directory.yaml.example
  • src/ingestion/silver/_shared/class_people.sql
  • src/ingestion/silver/_shared/identity_inputs.sql
🚧 Files skipped from review as they are similar to previous changes (14)
  • src/ingestion/silver/_shared/class_people.sql
  • src/ingestion/connectors/hr-directory/active-directory/source_active_directory/init.py
  • src/ingestion/connectors/hr-directory/active-directory/Dockerfile
  • src/ingestion/secrets/connectors/active-directory.yaml.example
  • src/ingestion/connectors/hr-directory/active-directory/dbt/active_directory__bronze_promoted.sql
  • src/ingestion/connectors/hr-directory/active-directory/pyproject.toml
  • docs/domain/connector/HR_IDENTITY_CONNECTOR_GUIDE.md
  • src/ingestion/silver/_shared/identity_inputs.sql
  • src/ingestion/connectors/hr-directory/active-directory/dbt/active_directory__users_snapshot.sql
  • src/ingestion/connectors/hr-directory/active-directory/dbt/schema.yml
  • src/ingestion/connectors/hr-directory/active-directory/dbt/active_directory__identity_inputs.sql
  • src/ingestion/connectors/hr-directory/active-directory/source_active_directory/spec.json
  • src/ingestion/connectors/hr-directory/active-directory/source_active_directory/ldap_client.py
  • src/ingestion/connectors/hr-directory/active-directory/descriptor.yaml

Comment thread src/ingestion/connectors/hr-directory/active-directory/README.md Outdated
cyberdima and others added 3 commits July 27, 2026 13:50
…ail fan-out

Address remaining CodeRabbit findings on PR #1900: join manager identity
resolution on tenant_id/source_id (not DN value alone) to prevent
cross-tenant leakage, dedupe simultaneous mail/userPrincipalName history
events, emit a DELETE when a manager loses its last email signal, and
correct the README privacy allowlist wording.

Signed-off-by: Dmitry Saukh <38005371+cyberdima@users.noreply.github.com>
@cyberdima
cyberdima merged commit 453df2d into main Jul 28, 2026
42 checks passed
mitasovr pushed a commit to mitasovr/insight that referenced this pull request Jul 30, 2026
… the class

The connector merged in constructorfabric#1900 was half-landed. Three defects, each of which
alone breaks a shared surface (issue constructorfabric#2048):

1. `descriptor.yaml` carried `version: "1.0"` — two components. The CI
   `bump-descriptors` job feeds image-bearing descriptors to
   bump-descriptor-version.sh, which hard-fails on non-semver. That job runs
   only on the push to main, so PR constructorfabric#1900 went green and the job aborted
   post-merge, leaving `images.cdk.image` empty and the connector
   undeployable (reconcile WARN+skips an empty ref).

   Fixed to "1.0.0" and pinned images.cdk.image to the image that run
   actually published and tagged latest:
   ghcr.io/constructorfabric/source-active-directory-insight:2026.07.28.06.32-453df2d

   Set directly rather than by rebuild: discover-image-matrix.py excludes
   descriptor.yaml from the trigger set, so a descriptor-only commit emits an
   empty matrix, `any=false`, and bump-descriptors skips itself — verified,
   no rebuild and no recursion.

2. No entry in scripts/bootstrap-db/connectors-config.yaml, so bootstrap-db
   never created bronze_active_directory. Both AD models failed
   `Code: 81 UNKNOWN_DATABASE`, and the hard `depends_on` in
   silver/_shared/class_people.sql cascaded that into the shared class:
   `PASS=202 ERROR=2 SKIP=6`, with silver.class_people and
   insight.metric_entity_cohorts_current absent. dbt's non-zero exit then
   tripped `set -e` before the gold-view migrations ran, so any regenerated
   connectors-ddl snapshot would silently lose both relations.

   Added the entry as a generated fragment
   (`./generate-connectors-config.sh 'hr-directory/active-directory'`) rather
   than regenerating the whole file, which would replace the HubSpot and
   Salesforce `env:` credential references with fake `value:` entries.

3. `manager_person_id` was `CAST(NULL AS Nullable(UUID))` while ms-entra uses
   `Nullable(String)` and bamboohr/workday project String columns.
   union_by_tag UNION ALLs all four branches, so fixing (2) alone would have
   swapped Code 81 for `Code: 386 NO_COMMON_TYPE`. `org_unit_id` is
   Nullable(UUID) in all four and is left alone.

Verified on a fresh ClickHouse 25.7.5: all four bootstrap-db steps run,
exit 0, dbt `PASS=210 ERROR=0 SKIP=0`, and silver.class_people,
insight.metric_entity_cohorts_current and bronze_active_directory.users all
exist. Compiled class_people unions all four HR sources, with
manager_person_id resolved to Nullable(String).

Prevention: scripts/ci/connector_wiring.py, wired into ci.yml as
`connector-wiring-guard`. It checks strict semver on image-bearing
descriptors, connectors-config.yaml registration, the class_<X> depends_on
edge, and cross-source column-type agreement. Run against the merge commit
453df2d it reports exactly these three errors and exits 1, so this PR would
have been blocked. Empty image refs and legacy non-semver versions on
image-less descriptors are warnings, not errors: a brand-new CDK connector
legitimately ships `image: ""`, and ADR-0015 §"Legacy non-semver values"
tolerates 2026.05.04 on ai/openai, collaboration/slack and
hr-directory/bamboohr — none of which declares an `images:` block, so none
can reach bump-descriptors.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Signed-off-by: Roman Mitasov <Roman.Mitasov@constructor.tech>
cyberantonz added a commit that referenced this pull request Jul 31, 2026
… the class (#2116)

The connector merged in #1900 was half-landed. Three defects, each of which
alone breaks a shared surface (issue #2048):

1. `descriptor.yaml` carried `version: "1.0"` — two components. The CI
   `bump-descriptors` job feeds image-bearing descriptors to
   bump-descriptor-version.sh, which hard-fails on non-semver. That job runs
   only on the push to main, so PR #1900 went green and the job aborted
   post-merge, leaving `images.cdk.image` empty and the connector
   undeployable (reconcile WARN+skips an empty ref).

   Fixed to "1.0.0" and pinned images.cdk.image to the image that run
   actually published and tagged latest:
   ghcr.io/constructorfabric/source-active-directory-insight:2026.07.28.06.32-453df2d

   Set directly rather than by rebuild: discover-image-matrix.py excludes
   descriptor.yaml from the trigger set, so a descriptor-only commit emits an
   empty matrix, `any=false`, and bump-descriptors skips itself — verified,
   no rebuild and no recursion.

2. No entry in scripts/bootstrap-db/connectors-config.yaml, so bootstrap-db
   never created bronze_active_directory. Both AD models failed
   `Code: 81 UNKNOWN_DATABASE`, and the hard `depends_on` in
   silver/_shared/class_people.sql cascaded that into the shared class:
   `PASS=202 ERROR=2 SKIP=6`, with silver.class_people and
   insight.metric_entity_cohorts_current absent. dbt's non-zero exit then
   tripped `set -e` before the gold-view migrations ran, so any regenerated
   connectors-ddl snapshot would silently lose both relations.

   Added the entry as a generated fragment
   (`./generate-connectors-config.sh 'hr-directory/active-directory'`) rather
   than regenerating the whole file, which would replace the HubSpot and
   Salesforce `env:` credential references with fake `value:` entries.

3. `manager_person_id` was `CAST(NULL AS Nullable(UUID))` while ms-entra uses
   `Nullable(String)` and bamboohr/workday project String columns.
   union_by_tag UNION ALLs all four branches, so fixing (2) alone would have
   swapped Code 81 for `Code: 386 NO_COMMON_TYPE`. `org_unit_id` is
   Nullable(UUID) in all four and is left alone.

Verified on a fresh ClickHouse 25.7.5: all four bootstrap-db steps run,
exit 0, dbt `PASS=210 ERROR=0 SKIP=0`, and silver.class_people,
insight.metric_entity_cohorts_current and bronze_active_directory.users all
exist. Compiled class_people unions all four HR sources, with
manager_person_id resolved to Nullable(String).

Prevention: scripts/ci/connector_wiring.py, wired into ci.yml as
`connector-wiring-guard`. It checks strict semver on image-bearing
descriptors, connectors-config.yaml registration, the class_<X> depends_on
edge, and cross-source column-type agreement. Run against the merge commit
453df2d it reports exactly these three errors and exits 1, so this PR would
have been blocked. Empty image refs and legacy non-semver versions on
image-less descriptors are warnings, not errors: a brand-new CDK connector
legitimately ships `image: ""`, and ADR-0015 §"Legacy non-semver values"
tolerates 2026.05.04 on ai/openai, collaboration/slack and
hr-directory/bamboohr — none of which declares an `images:` block, so none
can reach bump-descriptors.



(cherry picked from commit 0ecce4f)

Signed-off-by: Roman Mitasov <Roman.Mitasov@constructor.tech>
Signed-off-by: Anton Zelenov <antonz@constructor.tech>
Co-authored-by: Roman Mitasov <Roman.Mitasov@constructor.tech>
Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
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