Skip to content

DXBE-19: Add SAS API client service - #2039

Open
phenaproxima wants to merge 3 commits into
acquia:mainfrom
phenaproxima:sas-client-service
Open

DXBE-19: Add SAS API client service#2039
phenaproxima wants to merge 3 commits into
acquia:mainfrom
phenaproxima:sas-client-service

Conversation

@phenaproxima

Copy link
Copy Markdown

Adds a dedicated client stack for the Sites Aggregation Service (SAS) API, a standalone Acquia service that shares the Cloud API auth layer (same key/secret, same OAuth token endpoint).

  • SasApi\SasCredentials extends CloudApi\CloudCredentials, inheriting key/secret/token resolution and owning only the base URI: ACLI_SAS_BASE_URI env var, defaulting to the prod SAS service URL.
  • SasApi\SasClientService extends CloudApi\ClientService, injected with a separately wired sas.connector_factory — same pattern as V3ClientService — so requests go to SAS rather than the Cloud API gateway.

No connector subclass is needed: the SDK Connector already implements the shared OAuth client-credentials flow, and SAS paths (/sites/...) never match PathRewriteConnector rules in MEO contexts.

This is groundwork for upcoming source:cms:* commands that will call SAS endpoints (e.g. POST /api/sites/{siteId}/config-sync), which are not part of the Cloud API OpenAPI specs and therefore need a hand-written client.

Introduce a dedicated client stack for the Sites Aggregation Service
(SAS) API, a standalone service that shares the Cloud API auth layer:

- SasApi\SasCredentials extends CloudApi\CloudCredentials, inheriting
  key/secret/token resolution and owning only the base URI
  (ACLI_SAS_BASE_URI; defaults to the prod SAS service URL).
- SasApi\SasClientService extends CloudApi\ClientService, injected with
  a separately wired sas.connector_factory (same pattern as
  V3ClientService) so requests go to SAS instead of the Cloud API
  gateway.

No connector subclass is needed: the SDK Connector handles the shared
OAuth flow, and SAS paths (/sites/...) never match PathRewriteConnector
rules in MEO contexts.
Copilot AI lite review requested due to automatic review settings August 24, 2026 19:03
@codecov

codecov Bot commented Aug 24, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 92.54%. Comparing base (9270dd3) to head (11739cb).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff            @@
##               main    #2039   +/-   ##
=========================================
  Coverage     92.54%   92.54%           
- Complexity     2022     2025    +3     
=========================================
  Files           126      128    +2     
  Lines          7307     7312    +5     
=========================================
+ Hits           6762     6767    +5     
  Misses          545      545           

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@github-actions

Copy link
Copy Markdown

Try the dev build for this PR: https://acquia-cli.s3.amazonaws.com/build/pr/2039/acli.phar

curl -OL https://acquia-cli.s3.amazonaws.com/build/pr/2039/acli.phar
chmod +x acli.phar

The service hostname includes a -prod suffix:
sites-aggregation-service-prod.prod.cicd.acquia.io.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Adds a dedicated SAS API client stack sharing Cloud API authentication.

Changes:

  • Adds SAS credentials and configurable base URI.
  • Adds SAS client service and production wiring.
  • Adds credential and client tests.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.

Show a summary per file
File Review notes
tests/phpunit/src/SasApi/SasClientServiceTest.php Moderate: Add PHPUnit’s serial group because tests mutate environment state.
tests/phpunit/src/SasApi/EnvVarSasAuthenticationTest.php Moderate: Add PHPUnit’s serial group because tests mutate environment state.
src/SasApi/SasCredentials.php Moderate: Treat an empty ACLI_SAS_BASE_URI value as unset and use the production default.
src/SasApi/SasClientService.php No final review comments.
config/prod/services.yml No final review comments.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread src/SasApi/SasCredentials.php Outdated
Comment thread tests/phpunit/src/SasApi/EnvVarSasAuthenticationTest.php
Comment thread tests/phpunit/src/SasApi/SasClientServiceTest.php
- Treat a set-but-empty ACLI_SAS_BASE_URI the same as unset.
- Put env-var-mutating SAS tests in the serial group per house rules.
@phenaproxima

phenaproxima commented Aug 24, 2026

Copy link
Copy Markdown
Author

Human-authored comment here.

I think this PR makes sense, given what I discovered from talking to @danepowell and @justafish:

  • SAS is an API distinct from Cloud API, so it needs its own distinct client to talk to SAS.
  • But since it shares an authentication layer with Cloud, minimally extending ACLI's Cloud API client makes sense.

This PR is agent-generated, but it follows preexisting patterns for Cloud API v3. I'm unfamiliar with the intricacies of those patterns, so that's why this is vibe-coded.

The next step is probably to test that I can actually have some sort of meaningful interaction with SAS via ACLI; a temporary dummy command might be the ticket there, using my staging account. But otherwise this is ready for review; I can answer for it on at least a basic level.

@phenaproxima phenaproxima changed the title Add SAS API client service DXBE-19: Add SAS API client service Aug 24, 2026

@danepowell danepowell left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I'm no longer on the DevX team and don't have any official authority over this, but IMHO this should not be accepted. ACLI should not be talking to any API other than Cloud API (the fact that it does also happen to talk to ACSF API is a legacy that we should move away from). The reference architecture for Cloud API v3 and beyond is for all services to be composed under the unified cloud.acquia.com/api gateway.

You should consult with the architecture and CXAPI teams to confirm this before making a decision.

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.

3 participants