Skip to content

feat(gooddata-sdk): [AUTO] Add DeclarativeIpAllowlistPolicy schema to declarative org config#1607

Open
yenkins-admin wants to merge 6 commits into
masterfrom
auto/openapi-sync-C009-20260512-r59682
Open

feat(gooddata-sdk): [AUTO] Add DeclarativeIpAllowlistPolicy schema to declarative org config#1607
yenkins-admin wants to merge 6 commits into
masterfrom
auto/openapi-sync-C009-20260512-r59682

Conversation

@yenkins-admin
Copy link
Copy Markdown
Contributor

Summary

Added CatalogDeclarativeIpAllowlistPolicy SDK wrapper for the new DeclarativeIpAllowlistPolicy schema and wired it into the organization service as get/put methods. The policies live on the DeclarativeOrganization object (no dedicated endpoint), so get uses get_organization_layout and put does a read-modify-write via set_organization_layout.

Impact: new_feature | Services: gooddata-metadata-client

Files changed

  • packages/gooddata-sdk/src/gooddata_sdk/catalog/organization/layout/ip_allowlist_policy.py
  • packages/gooddata-sdk/src/gooddata_sdk/catalog/organization/service.py
  • packages/gooddata-sdk/src/gooddata_sdk/__init__.py
  • packages/gooddata-sdk/tests/catalog/test_catalog_organization.py

Agent decisions

Decisions (3)

No dedicated API endpoint — Use get_organization_layout / set_organization_layout (read-modify-write for puts)

  • Alternatives: Expose only a get method since there is no targeted PUT endpoint, Add a dedicated endpoint via _do_post_request
  • Why: The OpenAPI spec has no dedicated /layout/ipAllowlistPolicies endpoint; the only way to mutate these policies is through the full DeclarativeOrganization PUT. Read-modify-write is the same pattern used internally by the org layout. Exposing both get and put gives callers a consistent declarative interface matching the existing notification-channel and identity-provider patterns.

Reuse existing identifier classes — Use CatalogDeclarativeUserGroupIdentifier and CatalogUserIdentifier from identifier.py

  • Alternatives: Create new wrapper classes specifically for IP allowlist policy user/group references
  • Why: CatalogDeclarativeUserGroupIdentifier already wraps DeclarativeUserGroupIdentifier and CatalogUserIdentifier already wraps DeclarativeUserIdentifier. Both match the API schema exactly. No duplication needed.

List field defaults — allowed_sources, user_groups, users all use attrs.field(factory=list)

  • Alternatives: Make them Optional[list] defaulting to None
  • Why: The API schema has no required marker on these list fields but the pattern for list fields in the SDK (e.g. CatalogDeclarativeNotificationChannel, CatalogDeclarativeUserGroup) uses factory=list so empty collection is the safe default.
Assumptions to verify (3)
  • DeclarativeIpAllowlistPolicy.allowed_sources and id are always present in an API response (they are required positional args in the generated client).
  • set_organization_layout is safe to use for targeted IP allowlist policy updates as long as the full DeclarativeOrganization is read first and sent back unmodified except for the ip_allowlist_policies field.
  • The _check_return_type=False flag causes the API client to still return a ModelNormal instance (not a raw dict), consistent with other layout API calls.
Risks (2)
  • test_layout_ip_allowlist_policies: will fail if the staging/test environment does not yet support ip_allowlist_policies in the DeclarativeOrganization response (API version mismatch).
  • put_declarative_ip_allowlist_policies: the read-modify-write involves two API calls; if the org layout is very large, the set_organization_layout PUT body could be heavy.
Layers touched (3)
  • declarative_model — New CatalogDeclarativeIpAllowlistPolicy class wrapping DeclarativeIpAllowlistPolicy
    • packages/gooddata-sdk/src/gooddata_sdk/catalog/organization/layout/ip_allowlist_policy.py
  • public_api — Export of new class; two new service methods added to CatalogOrganizationService
    • packages/gooddata-sdk/src/gooddata_sdk/__init__.py
    • packages/gooddata-sdk/src/gooddata_sdk/catalog/organization/service.py
  • tests — Integration test test_layout_ip_allowlist_policies added
    • packages/gooddata-sdk/tests/catalog/test_catalog_organization.py

Source commits (gdc-nas)

  • f36f74d Merge pull request #22737 from gooddata/dnik/md-allowlist
OpenAPI diff
@@ gooddata-metadata-client.json @@
+      "DeclarativeIpAllowlistPolicy": {
+        "description": "A declarative form of an IP allowlist policy.",
+        "properties": {
+          "allowedSources": { "items": { "type": "string" }, "type": "array" },
+          "id": { "pattern": "^(?!\\.)[.A-Za-z0-9_-]{1,255}$", "type": "string" },
+          "userGroups": { "items": { "$ref": "DeclarativeUserGroupIdentifier" }, "type": "array" },
+          "users": { "items": { "$ref": "DeclarativeUserIdentifier" }, "type": "array" }
+        }
+      }
       "DeclarativeOrganization": {
+          "ipAllowlistPolicies": { "items": { "$ref": "DeclarativeIpAllowlistPolicy" }, "type": "array" }
       }

Workflow run


Generated by SDK OpenAPI Sync workflow

Copy link
Copy Markdown
Contributor

@tychtjan tychtjan left a comment

Choose a reason for hiding this comment

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

There are transcripts in the commit and also unit tests are failing. Please fix

@yenkins-admin
Copy link
Copy Markdown
Contributor Author

✅ Review fixes applied

Note

Addressed feedback from @tychtjan in 4aa0d56.
Workflow run

Note

📼 Recording cassettes against staging — a follow-up commit will land shortly:

  • packages/gooddata-sdk/tests/catalog/test_catalog_organization.py::test_layout_ip_allowlist_policies

Invalidating cassettes for:

  • packages/gooddata-sdk/tests/catalog/test_catalog_organization.py::test_layout_ip_allowlist_policies

CI will be red until recording lands. That's expected.

Reply to review summary

Fixed both issues:\n\n1. Transcripts removedtranscripts/fix-C009-attempt1.jsonl and transcripts/fix-C009-attempt2.jsonl were left behind by the previous fix-agent runs. Both deleted via git rm.\n\n2. Test fixed and cassette replaced — The cassette was hand-crafted by the fix agents and contained multiple HTTP 400 responses because the test used non-existent entities (employee123 user, group.admins user-group). Updated the test to reference entities that actually exist in the staging environment (demo user, adminGroup user-group) and deleted the invalid cassette so the recorder pipeline can record a clean one.

Actions

2 fixed

✅ Fixed (2)
  • transcripts/fix-C009-attempt1.jsonl — Removed two accidentally committed transcript files (transcripts/fix-C009-attempt1.jsonl and transcripts/fix-C009-attempt2.jsonl) via git rm. These were agent conversation logs that the previous fix-agent runs committed by mistake.

  • packages/gooddata-sdk/tests/catalog/test_catalog_organization.py — The test_layout_ip_allowlist_policies integration test was failing because the hand-crafted cassette recorded multiple HTTP 400 Bad Request responses. Root cause: the test used non-existent entities — user employee123 and group group.admins — that the staging server rejected. Fixed by: (1) changing the test to use entities that exist in the staging environment (adminGroup / demo), updating the corresponding assertions; (2) deleting the hand-crafted cassette with git rm so the recorder pipeline can record a proper one against staging.

Claude output available in workflow artifacts.

@yenkins-admin
Copy link
Copy Markdown
Contributor Author

Warning

📼 ⚠️ Cassette recording failed after 2 fix-agent retry(ies).

Tests that did not record:

  • packages/gooddata-sdk/tests/catalog/test_catalog_organization.py::test_layout_ip_allowlist_policies

Last pytest output:

E   Content-Type: application/problem+json
E   Referrer-Policy: no-referrer
E   X-Gdc-Trace-Id: 67569c7cc76a54e3bc79b66c0a203c2b
E   X-Xss-Protection: 1; mode=block
E   Vary: Accept-Encoding
E   Vary: Origin
E   Vary: Access-Control-Request-Method
E   Vary: Access-Control-Request-Headers
E   Content-Length: 330
E   Cache-Control: no-cache, no-store, max-age=0, must-revalidate
E   
E   
E   HTTP response body: {"detail":"Invalid combination of auth properties. Only none, user + password, token, key-pair or client id + secret are allowed. Datasource: demo-test-ds, user: <yes>, password: <no>, token: <no>, key-pair: <no>, clientId: <no>, clientSecret: <no>","status":400,"title":"Bad Request","traceId":"67569c7cc76a54e3bc79b66c0a203c2b"}
---------------------------- Captured log teardown -----------------------------
WARNING  tests.conftest:conftest.py:294 Test failures detected — restoring session snapshot...
--------------------------------- JSON report ----------------------------------
report saved to: .json-report-py312.json

---------- coverage: platform linux, python 3.12.13-final-0 ----------
Coverage XML written to file coverage.xml

=========================== short test summary info ============================
FAILED tests/catalog/test_catalog_organization.py::test_layout_ip_allowlist_policies
============================== 1 failed in 7.16s ===============================
py312: exit 1 (8.08 seconds) /home/runner/_work/gdc-nas/gdc-nas/sdk/packages/gooddata-sdk> pytest -v --cov --cov-report=xml tests tests/catalog/test_catalog_organization.py::test_layout_ip_allowlist_policies --tb=short -q --json-report --json-report-file=.json-report-py312.json pid=2870
  py312: FAIL code 1 (8.43=setup[0.34]+cmd[8.08] seconds)
  evaluation failed :( (8.44 seconds)
make[1]: *** [../../project_common.mk:72: test-staging] Error 1
make[1]: Leaving directory '/home/runner/_work/gdc-nas/gdc-nas/sdk/packages/gooddata-sdk'
make: *** [Makefile:101: test-staging] Error 2

Workflow run logs
Manual recording probably needed: re-run via gh workflow run sdk-py-review-fix.yml -f pr_number=1607 -f pr_branch=<branch>.

Review cost so far: $1.2025.

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