DXBE-8: Add source:link, source:unlink, source:cms:config:pull and source:cms:config:push commands - #2035
DXBE-8: Add source:link, source:unlink, source:cms:config:pull and source:cms:config:push commands#2035phenaproxima wants to merge 34 commits into
source:link, source:unlink, source:cms:config:pull and source:cms:config:push commands#2035Conversation
Adds a new source:config:push command that assembles the config files under .acquia/config into a single YAML payload (keyed by config collection, then config name) and POSTs it to the Sites Aggregation Service (SAS), polling the resulting async operation until completion. Introduces a SasApi client layer modeled on the existing AcsfApi pattern. Because SAS shares the Accounts authentication layer with the Cloud API, the connector reuses the standard OAuth2 client-credentials token flow; only the base URI is new (ACLI_SAS_API_BASE_URI). Open items are marked with @todo DXBE-20: the SAS endpoint path and response field names are placeholders pending the SAS endpoint being built, and the payload may need to be JSON-encoded if the SAS team requires it.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2035 +/- ##
============================================
+ Coverage 92.76% 93.11% +0.35%
- Complexity 2032 2099 +67
============================================
Files 126 133 +7
Lines 7337 7526 +189
============================================
+ Hits 6806 7008 +202
+ Misses 531 518 -13 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Adds a new source:config:push Symfony Console command to package local .acquia/config/**/*.yml into a single “collections → config name → values” document and submit it to a new SAS (Sites Aggregation Service) API client layer, with polling for async completion. This fits alongside the existing Cloud/Acsf client patterns and command set.
Changes:
- Introduces
source:config:pushcommand, payload assembly, confirmation prompt, and async polling. - Adds a new
SasApi/client layer (connector, client service, endpoint wrapper) and wires it into the production service container. - Adds unit tests for payload assembly behavior (collections and empty cases).
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/phpunit/src/Commands/Source/ConfigPushCommandTest.php | Adds unit tests for config payload assembly from .acquia/config directory structure. |
| src/Command/Source/ConfigPushCommand.php | Implements the source:config:push command: reads YAML config files, submits to SAS, and polls operation status. |
| src/SasApi/SourceConfig.php | Defines SAS endpoint wrapper methods for submitting a config push and checking operation status. |
| src/SasApi/SasCredentials.php | Provides SAS base URI configuration (env var + default). |
| src/SasApi/SasConnectorFactory.php | Adds a connector factory for SAS requests. |
| src/SasApi/SasConnector.php | Adds a SAS connector class extending the Cloud API connector. |
| src/SasApi/SasClientService.php | Adds a client-service factory for producing configured SasClient instances. |
| src/SasApi/SasClient.php | Adds a SAS client class extending the Cloud API client. |
| config/prod/services.yml | Registers SAS credentials + connector factory wiring and excludes SourceConfig from auto-service registration (instantiated manually by the command). |
Suppressed comments (2)
src/Command/Source/ConfigPushCommand.php:93
execute()currently callsdetermineSiteInstance(), which only returns a value when--siteInstanceIdis provided (seeCommandBase::determineSiteInstance()), so the command will always throw in the default “no args” flow and the[environmentId]argument is effectively ignored. UsingCommandBase::determineEnvironment()here would allow the documented environment resolution (including git-remote inference) while still supporting--siteInstanceIdas an override.
$siteInstance = $this->determineSiteInstance($input);
if ($siteInstance === null) {
throw new AcquiaCliException(
'Could not determine a Source site instance. Run this command from a repository linked to an Acquia Cloud application, or pass --siteInstanceId.'
);
src/Command/Source/ConfigPushCommand.php:157
Yaml::parseFile()will throw on invalid YAML and can return non-array values; right now that bubbles up as an unhandled exception or produces an invalid payload shape. Consider catching YAML parse failures per-file and throwing anAcquiaCliExceptionthat includes the offending path, and validate that each config file parses to an array/map.
$collection = $relativeDir === '' ? '' : str_replace('/', '.', $relativeDir);
$name = $file->getBasename('.yml');
$payload[$collection][$name] = Yaml::parseFile($file->getPathname());
}
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| protected function execute(InputInterface $input, OutputInterface $output): int | ||
| { | ||
| $this->setDirAndRequireProjectCwd($input); | ||
|
|
There was a problem hiding this comment.
Fair point on the convention, but we're deliberately holding off on an execute()-level test for now: the SAS endpoint doesn't exist yet, so any test would just cement a placeholder request/response shape we'd have to redo once the real API lands. The payload assembly (the novel logic) is covered. We'll add command-level coverage once the endpoint's contract is settled — tracked as part of DXBE-20.
Mirror the Cloud API ConnectorFactory: fall back to an AccessTokenConnector when a valid access token is present (e.g. a bot token in CI), instead of always building a key/secret connector. Also widen the SasConnector config phpdoc to allow nullable values.
|
Try the dev build for this PR: https://acquia-cli.s3.amazonaws.com/build/pr/2035/acli.phar |
CodebaseEnvironmentResponse exposes ->id, unlike EnvironmentResponse which uses ->uuid. Passing the wrong property would have errored at runtime.
The SAS endpoint triggers drush source:config:import, which reads config from the site's deployed git repository. The acli command is a thin trigger: resolve the site instance, POST with an empty body, poll. Remove the payload-assembly logic and its tests, which are no longer needed.
Adds source:config:pull, a mirror of push that triggers a SAS config export (CMS to repo). Extracts the shared trigger/poll flow into an abstract ConfigCommandBase so both directions reuse the same SAS client wiring; each subclass supplies only its endpoint call and messaging. Generalizes SourceConfig::getStatus() to serve both operations.
Pull now fetches the exported YAML payload after the operation completes and writes it to .acquia/config/, wiping and rewriting the directory so local files mirror the remote state exactly (config removed in the CMS disappears locally). Collections map to directories (default '' is the root; language.es becomes language/es). Adds unit tests for the writer. Adds SourceConfig::getExportPayload() to retrieve the YAML, and reworks ConfigCommandBase with an onSuccess() hook so pull can write files after a successful operation while push stays trigger-only.
Mock the Cloud API site-instance resolution chain and the SAS client to exercise the full execute() path for both push and pull: trigger, poll, and (for pull) payload fetch and write. Endpoint shapes are placeholders (@todo DXBE-20) to be re-pointed once the real SAS endpoint lands.
Add unit tests covering previously-escaped mutants: SasConnectorFactory connector selection (key/secret vs valid/expired token vs none), SasConnector base-URI passthrough, and SasClientService construction. Strengthen the push execute() test to assert exact status output, and add a nested-structure writer test to kill the Yaml::dump depth/indent mutants. Mark the transient spinner-message concat as infection-ignored (it never appears in captured output, so it cannot be asserted).
Rework the non-array-collection test so the malformed entry is iterated between two valid collections, catching a continue-to-break mutation while staying alphabetical for the code-style fixer. Add an empty-payload test to kill the mkdir-removal mutant. Mark two genuinely unobservable framework-glue mutants (onSuccess visibility, configureClient headers) as infection-ignored with justification.
Extract the Yaml::dump magic numbers into named constants and mark the unobservable depth increment/decrement as infection-ignored. Fix the access-token factory test to assert the connector type rather than the token value (the existing Cloud code nests the token object, a pre-existing quirk not worth depending on).
Add partial-credential-plus-token cases so a flipped key/secret condition (&& mutated to ||, or a negated operand) routes to the wrong branch and fails the test. This makes the factory's auth-selection logic observable at the unit level.
The key/secret branch and the unauthenticated fallback both return a SasConnector, so removing the branch's return produced an identical type. Distinguish them by the connector's private clientId: 'k' on the authenticated path, null on the fallback.
Both the key/secret branch and the unauthenticated fallback construct a SasConnector from the same config, so removing the branch's return yields an externally identical object. Mark it infection-ignored with justification, and remove the reflection-based test that could not distinguish the branches. Local Infection reports 100% MSI.
This AFAICT needs both |
|
This PR is correctly hard-blocked on SAS + SAT PRs (see prior comment), but AFAICT neither PR currently does enough to allow this PR to work: https://github.com/acquia/sites-aggregation-service/pull/971#pullrequestreview-4960954310 |
| */ | ||
| public function push(string $environmentId): object | ||
| { | ||
| return $this->client->request('post', "/environments/$environmentId/config-import"); |
There was a problem hiding this comment.
acquia/sites-aggregation-service#971 specifies POST /api/sites/{siteId}/config-sync, authorised on site admin for the site. This sends POST /environments/{environmentId}/config-import — wrong resource and wrong path.
determineSiteInstance() already resolves $siteInstance->site (CommandBase.php:895), but execute() keeps only the environment (line 112). Pass $siteInstance->site->id.
| // @todo DXBE-20: Confirm the operation ID field name with the SAS team. | ||
| $operationId = $response->id ?? null; |
There was a problem hiding this comment.
acquia/sites-aggregation-service#971 returns 202 Accepted with a Message and no id, so this throws an exception on every invocation. testExecuteThrowsWhenOperationIdMissing() is currently testing the production path.
Needs upstream first, then still changes here. Point 2 of my review on acquia/sites-aggregation-service#971 asks for config-sync to create a notification. If it does, the identifier comes off that notification rather than off the 202 body, so this block still has to be rewritten — it just stops being unfixable.
| * @todo DXBE-20: Confirm the status field name and its values with the | ||
| * SAS team. Assumes a `status` field mirroring the task gateway's | ||
| * phases (pending/running/succeeded/failed). | ||
| */ | ||
| private function waitForOperation(SourceConfig $sourceConfig, string $operationId): bool |
There was a problem hiding this comment.
GET /config-operation/{id} doesn't exist. acquia/sites-aggregation-service#971's description: "No notification record is created and no status polling is wired up, so the caller gets a 202 and no way to observe the outcome beyond the task log."
So getStatus(), waitForOperation() and the pending/running/succeeded/failed states are all guesses.
There is a real surface to aim at, though. SAS already exposes GET /sites/{siteId}/notifications, with status, progress and completed_at on each Notification — and Site::createBackup(), restoreBackup() and export() all create one. syncConfig() is the exception. So the shape is polling notifications once config-sync creates one, not a bespoke /config-operation/{id} resource.
Needs upstream first, then still changes here. Point 2 of my review on acquia/sites-aggregation-service#971 decides which: if config-sync creates a notification, this polls that instead of /config-operation/{id}; if fire-and-forget is the intended contract, this method comes out entirely. Either way the code changes.
| * @todo DXBE-20: Confirm the endpoint paths and response field names with the | ||
| * SAS team. The SAS endpoints do not exist yet; paths here are placeholders. | ||
| */ | ||
| class SourceConfig extends CloudApiBase | ||
| { | ||
| /** | ||
| * Trigger a config import on a site environment (repo to CMS). | ||
| * | ||
| * @return object The decoded response, expected to contain an operation ID. | ||
| */ | ||
| public function push(string $environmentId): object | ||
| { | ||
| return $this->client->request('post', "/environments/$environmentId/config-import"); | ||
| } |
There was a problem hiding this comment.
DXBE-19 asks for the current folder of config to be sent as the POST body. Per acquia/haas-drupal#2233, dr source:config:import --single-yaml reads exactly that from stdin — and the assembly this PR originally had (keyed by collection, then config name) matched it.
Commit e44a7a1 deleted that to match acquia/sites-aggregation-service#971's input: false. The result is a push that sends nothing from the developer's machine and imports whatever is already deployed. So pull → edit → push silently ignores local edits until they're committed and deployed.
Needs upstream first, then still changes here. Point 1 of my review on acquia/sites-aggregation-service#971 asks for an optional payload; if it lands, the deleted assembly has to be restored in this PR. If the answer is that the payload path is out of scope for DXBE-20, then DXBE-19 needs revisiting rather than this code.
| $filesystem = new Filesystem(); | ||
|
|
||
| // Wipe the directory so the local files mirror the remote state. | ||
| $filesystem->remove($configDir); |
There was a problem hiding this comment.
Pull is DXBE-8 scope, so not out of place — but acquia/sites-aggregation-service#971 adds only the import direction, so config-export and /payload have nothing to run against.
Not covered by my review on acquia/sites-aggregation-service#971 either — that asks about the push payload, not a config export endpoint. POST /sites/{siteId}/exports does exist, but it's a whole-site export (MysqlSiteExportAdapter), not CMS config. The export endpoint is DXBE-21, so this half is blocked on that landing first.
That matters because pull is the half that deletes files. testWritePayloadCreatesConfigDirWhenPayloadEmpty() shows it as a passing test: {} parses to [], passes the is_array() guard at line 65, wipes .acquia/config/ and writes nothing back. Any SAS-side bug returning an empty export destroys uncommitted local config, and the prompt says nothing about local deletion.
Suggest splitting pull into its own PR. If it stays: name the directory in the prompt, write to a temp dir and swap, and refuse to wipe on an empty payload.
| { | ||
| $this->setDirAndRequireProjectCwd($input); | ||
|
|
||
| $siteInstance = $this->determineSiteInstance($input); |
There was a problem hiding this comment.
The PR description says site identity is resolved from the git remote with no arguments needed. determineSiteInstance() (CommandBase.php:884-906) reads only --siteInstanceId and otherwise returns null, so line 106 throws unless that option is passed. acceptEnvironmentId() (line 69) registers an argument nothing reads.
CommandBase::determineEnvironment() (CommandBase.php:690-708) is the existing ladder: --siteInstanceId → environmentId argument → codebase environment → git-remote-matched application → prompt. It also normalises through EnvironmentTransformer, which would make commit 4768081 unnecessary.
All six execute() tests pass --siteInstanceId, which is why this wasn't caught.
| protected function operationLabel(): string | ||
| { | ||
| return 'Importing configuration'; | ||
| } |
There was a problem hiding this comment.
operationLabel() is the whole of the push wording, and ConfigCommandBase.php:116 renders it as Importing configuration on the <env> environment?. Per acquia/sites-aggregation-tasks#261, confirming it puts the site in maintenance mode, backs up the database, checkpoints config, imports, and on failure reverts the checkpoint or restores the database. acquia/sites-aggregation-service#971 opens its risk section with "This triggers a destructive operation on a live site … The site is offline for the duration."
Someone pointing this at production can't tell from that sentence that the site goes down. EnvMirrorCommand.php:61 and PushDatabaseCommand.php:48 are the models — they name what gets overwritten. Please say that the site goes into maintenance mode and is unavailable while this runs, and that a database backup is taken first.
Anchoring here because the warning is push-specific: pull doesn't take the site offline. But the base's sprintf('%s on the %s environment?', ...) template only has room for a short label, so either that grows or the base needs a hook for the extra warning text.
| $json = $this->outputsJson(); | ||
| $root = $this->workingCopyDir(); | ||
| $siteId = $this->determineSourceSite($root); | ||
| $response = $this->cloudApiClientService->getClient()->request('get', "/source-sites/$siteId/config"); |
There was a problem hiding this comment.
👍 This correctly does not expect export to ever fail, because upon export, config is not validated: https://github.com/acquia/product-specs/pull/214/changes/bf4674abf6e766de9ac09567846b805614322866.
|
Ready for @phenaproxima to review! Left 3 review pointers:
CI fails for unrelated reasons with: … which is also happening on |
PR 2035: Source Config Sync Commands — Revised ReviewStateDraft, blocked on upstream; 14 lines missing test coverage on a patch that's 89.4% covered. What this deliversThree new commands wrapping Cloud API v3 to move Source site configuration between the live site and a working copy ( The commands' core logic is sound and well-tested. The blocking issue is not the code — it's that three upstream services don't exist yet, so the endpoints it calls will 404 when deployed. Upstream blockers (real, not code bugs)Three services this PR depends on aren't deployed yet:
Until those three land, nothing runs end to end. The tests don't expose this because they mock the responses. This is the right approach — building against the spec while the service catches up — but it means the commands are shipped untested against reality. Code quality (better than the original review stated)The logic is correct where it runs:
Legitimate code issues (architectural, not critical)Empty subclasses. Dead branch in SasConnectorFactory. Lines 30 and 46 return the same object ( Unused interface methods. Hardcoded SAS base URI. The code uses Test coverage and gaps89.4% coverage on patch; 14 lines uncovered. Mostly in Command tests don't exercise site resolution. Every push/pull test passes Pull tests don't verify response structure. The test mocks a response with Push tests correctly cover the paths: 409 conflict, async polling, all three status outcomes (succeeded, refused, failed), invalid YAML errors, missing Confirmation prompt wordingThe prompt says: "Replace the configuration of Source site $siteId with the contents of $configDir? The site is put in maintenance mode and its database is backed up first." This does warn that the site goes into maintenance mode. It could be more explicit (e.g., "The site will be offline..."), but it's not misleading. The comparison to Feature completeness vs. tickets
What's missing from scope
VerdictThe code works. The three commands implement their spec correctly. Path traversal is prevented. Interactive vs. non-interactive behavior is correct. Polling is robust. Fixtures are complete and authentic. The blocking issues are all upstream: the export endpoint doesn't exist, the status resource doesn't exist, and the import notification contract is still being clarified. This PR correctly assumes the spec and builds testable logic against mocked responses. The legitimate cleanup items are: delete the empty subclasses, fix the dead connector branch, clarify or make configurable the SAS base URI. None of these are critical bugs; all are addressable in small followup commits. Worth landing once the three upstream services deploy, but only after fixing the dead connector branch (not just ignoring it) and settling on the SAS base URI. The empty subclasses can wait until SAS decides if they'll need overrides in the future. |
|
Note This comment was written by Claude Haiku 4.5 with adversarial review by Claude Opus 5. The process is documented below. Process noteThis review started as an initial assessment, then underwent adversarial review by Claude Opus 5 to catch errors and overstatements. The Opus review identified five significant factual errors in the initial review:
The revised review reflects these corrections and focuses on real issues: upstream service gaps (DXBE-21, DXBE-100), legitimate architectural cleanups (dead connector branch, hardcoded URI), and feature gaps that belong to different tickets (configurable directory, revert path). Models and tools
This two-pass process caught errors that a single review pass would have missed. |
Quick note on
|
Summary for @phenaproximaThree comments posted:
All three commands implement their spec correctly. The code is production-ready except for the three upstream service gaps (export endpoint, status resource, notification contract). Worth merging once those land, after fixing the dead connector branch and settling the SAS base URI. |
Review №1, titled "PR 2035: Source Config Sync Commands — Revised Review"Upstream blockers (real, not code bugs)✅ Correct. Clarifications:
Legitimate code issues (architectural, not critical)❌ Wrong.
This is nonsense. This tells me this AI-generated review was applied to the state of the PR before yesterday, because all that was deleted in 48a98cb 😅 Confusingly, the rest of the review is talking about things that were only introduced yesterday (e.g. I'm baffled how both can be true at the same time. Feature completeness vs. tickets❌ Wrong.
👎 False: see the comments on DXBE-8.
👎 False: see the comments on DXBE-8. What's missing from scope❌ Wrong. After the above two 👎s, nothing is missing. This is a whole new level of AI slop, and you should've caught that, @phenaproxima 😞 Review №2, titled "written by Claude Haiku 4.5 with adversarial review by Claude Opus 5. The process is documented below."🤔 Was review №1 written by Haiku, and this one by Opus? I don't understand what this comment is stating. It seems to state that … Haiku found problems, Opus refuted them all? Review №3, titled "Quick note on
|
…sh confirmation Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
The only thing that I can address from the 1804 words (!) in the 4 @phenaproxima review comments is this:
Done: 6fad178 |
…-push * origin/main: CLI-1837: remote:ssh should accept environmentId (acquia#2046)
I addressed my own review in yesterday's complete overhaul.
Symfony Console writes PHP_EOL per line (\r\n on Windows), so CommandTestBase::getDisplay() now normalizes it to \n for the whole suite's assertions. SourceConfigDocument::toFiles() always joins collection directories with '/', but Finder::getRelativePathname() returns the OS separator, so SourceConfigDocumentTest normalizes its fixture paths too. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
I am really struggling with how to use AI for review. It's not clear to me how that's even supposed to work. This is a human activity, and if I'm just using AI to review, what the hell is left for me to do? I hate AI-driven development, and I don't care who knows it. The AI era sucks like a black hole. Nonetheless, @wimleers put effort into this and it therefore deserves a real human review. So I will re-review it manually today, and please accept my apologies for this slop. I don't feel good about it. |
| @@ -0,0 +1,43 @@ | |||
| uuid: 54eeac89-87fa-4a96-8072-f0ede31bec0a | |||
There was a problem hiding this comment.
Not in scope here, but I'm surprised we export this resource type. Search API tasks are internal entities that are used to track work Search API has to do, and it's unlikely anyone would want to access them through JSON:API.
(And more to the point, not sure they should; what if you maliciously injected or updated tasks in such a way that it messes up the site, or causes it to do a lot of extra work to waste resources?)
There was a problem hiding this comment.
GREAT catch! 👏 You're right; this is actually prevented by you discovered a bug/oversight in DXBE-67. This PR's fixture generation simply was wrong!DXBE-67.
There was a problem hiding this comment.
→ being tackled in https://acquia.atlassian.net/browse/DXBE-67?focusedCommentId=10688293 👍
There was a problem hiding this comment.
This is merge-blocking. Once that DXBE-67 bug is fixed, this PR will be able to just re-run the export 👍
| $this->io->error("Source site $siteId refused the configuration; nothing was imported:"); | ||
| foreach ($import->violations ?? [] as $violation) { | ||
| $location = match (true) { | ||
| isset($violation->collection) => "$violation->collection: $violation->config", |
There was a problem hiding this comment.
🤔 Will the default collection be NULL, or an empty string (which it is actual value)? If it's the latter, then this arm will always execute.
There was a problem hiding this comment.
Good Q, currently in flux; see https://acquia.atlassian.net/browse/DXBE-91 + https://acquia.atlassian.net/browse/DXBE-110. Holding off responding to this until that's resolved.
This is merge-blocking.
1. Rename --force to --yes: "force" implied a destructive remote overwrite, which this isn't (server-side validation still applies). 2. Move the config-directory scan into SourceConfigDocument::fromDirectory(), encapsulating the Finder usage instead of leaving it in the command. 3. Rename assertSafe() to assertPathIsSafe() and document its parameters, since it's the path-traversal guard. 4. Comment why LoopHelper::getLoopy() gets an empty done callback: the outcome is read from $import/$error by reference once it returns. Also converts ConfigPushCommandTest::DOCUMENT to heredoc syntax, per the same review. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@phenaproxima asked (r4016063860) what "ponytail" meant in the source:link pagination comment; it was leftover internal tooling jargon. Reworded to plain English. (Also considered adding Yaml::PARSE_CONSTANT for the 11.5 !php/const/!php/enum tags raised on r4007825810, but that's not actually safe: those tags resolve via \defined()/\constant(), which autoloads whatever class the tag names, and a real tag would reference a class from the Source site's own Drupal codebase, never autoloadable from acli's standalone process. It can only ever fail (same as today) or, on a name collision, resolve to an unrelated local value. haas-drupal's ConfigExportController::buildExportStorage() also never puts a PHP enum into what it exports, so there is nothing on the export side that would produce these tags anyway.) Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@phenaproxima raised this on r4016015166, framed as possible follow-up material; tackling it here instead since it mirrors app:unlink directly. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
wimleers
changed the title
source:link, source:cms:config:pull and source:cms:config:push commandssource:link, source:unlink, source:cms:config:pull and source:cms:config:push commands
r4007825810 asked about the decode() flags. \Drupal\Component\Serialization\Yaml::decode() passes Yaml::PARSE_CUSTOM_TAGS, added for services.yml's !tagged_iterator (https://www.drupal.org/node/3436859), and could reasonably also pass Yaml::PARSE_CONSTANT, for !php/const/!php/enum in config and service YAML (https://www.drupal.org/node/3403883). Neither applies here: what this pulls is built from plain config storage reads plus three literal system.site strings, never a tagged or enum value, so decode() doesn't need to accept either. PARSE_CONSTANT would also be actively unsafe to add: a !php/const tag would name a class from the Source site's own Drupal codebase, never autoloadable from acli's standalone process, so it could only fail or resolve to an unrelated value from this machine. Parsing either tag without its flag throws a clear ParseException, which is correct for config that should never contain one. Documented this on decode() itself, with @see references to \Drupal\Core\Config\FileStorage and \Drupal\Component\Serialization\Yaml — the actual Drupal codebase this is meant to match byte for byte, so a future reader can check both sides directly instead of trusting the commit message. The one existing test exercising a custom tag was synthetic, not drawn from the real fixture (grep confirms zero tags in tests/fixtures/source-config); replaced it with a rejection test covering both tag forms. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The full-command-list snapshot test never got the new command added. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
ℹ️ Proposed future docs to be published at https://docs.acquia.com/acquia-cli/ now in the PR body 😊 Note the use of the native |
|
Didn't do a full re-read, but I did look at the commits since my last read-through, and I'm happy with this. This is merely awaiting resolution of the merge-blocking comments (#2035 (comment) and #2035 (comment)). |
Reports the site's latest import (GET /source-sites/{id}/config/import)
without starting one: no confirmation, no .acquia/config requirement, no
PUT. Reuses reportImport()'s existing succeeded/refused/failed reporting,
adding the import's started_at (per api-specs' SourceSiteConfigImport
schema, since nothing hands back an import's id when it starts, so
started_at is what lets a caller tell a reported import apart from its
own) and a distinct exit code for "running" so a CI/CD run can poll on it
without misreading in-progress as failure.
Extracted reportResult() so the json-output silencing and the trailing
json_encode aren't copy-pasted between the --status branch and the
existing push flow.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
New commit (ba0d14b), based on @justafish's "import/push status?" feedback: |
Adds four commands for moving a Source site's configuration between the live site (served by Acquia) and a working copy (a local directory containing
.acquia/config, possibly managed in version control):acli source:link [<sourceSiteId>].acquia-cli.yml, likeapp:linkdoes for a Cloud applicationacli source:unlinkapp:unlinkacli source:cms:config:pullGET /source-sites/{id}/configon Cloud API v3, splits the returned document into.acquia/config/, one file per configuration object, replacing the directoryacli source:cms:config:push.acquia/config/back into one document,PUTs it, then pollsGET /source-sites/{id}/config/importuntil it issucceeded,refused(every violation is printed) orfailed; the exit code followsEndpoints used, all on Cloud API v3 (
https://api.acquia.com/v3,ACLI_CLOUD_API_V3_BASE_URIto override) with the same credentials every otheraclicommand uses:source:linkGET /source-sites(chooser, interactive only) andGET /source-sites/{sourceSiteId}getSourceSites,findSourceSiteBySourceSiteIdsource:cms:config:pullGET /source-sites/{sourceSiteId}/configfindSourceSiteConfigBySourceSiteIdsource:cms:config:pushPUT /source-sites/{sourceSiteId}/config, thenGET /source-sites/{sourceSiteId}/config/importsaveSourceSiteConfigBySourceSiteId,findSourceSiteConfigImportBySourceSiteIdThe working copy is the directory containing
.acquia/config; the commands work from any subdirectory of it. A site is given with--siteor taken from.acquia-cli.yml; nothing is guessed. Push asks for confirmation when interactive and requires--yesotherwise;--format=jsonprints the import outcome as JSON.The one piece of logic
acliowns is the conversion between the document and the directory (SourceConfigDocument). Files are written with the same YAML dumper settings Drupal core uses, andtests/fixtures/source-configholds a document a site emitted next to the files the site itself writes for it; the tests check both directions byte for byte.That fixture is most of this diff:
tests/fixtures/source-config/alone is 131 files and 11,732 lines. It is the complete configuration of a freshly installed Source site, written by the site's own configuration storage, not a hand-picked subset: the point is that a working copyacliwrites is indistinguishable from one the site writes, and only the site's full output proves that for every configuration type in scope. The README there records how to regenerate it when the scope changes.Caution
Two blockers:
search_api_taskfixture is merge-blocking: it's a real export bug tracked in DXBE-67, not a mistake in this PR's fixture generation. Once fixed, re-running the fixture export (seetests/fixtures/source-config/README.md) should drop the file on its own.Note
History: the first 15 commits are @phenaproxima's original draft against placeholder endpoints; the commits after the merge of
mainreplace the hand-written service client with the existingV3ClientService, rename the commands, add the transform andsource:link/source:unlink, and drop two decode() flags (Yaml::PARSE_CUSTOM_TAGS/PARSE_CONSTANT) that core's own YAML class carries for unrelated reasons.Not yet done
Proposed docs for
https://docs.acquia.com/acquia-cli/https://dev.acquia.com→ moved to DXBE-114's https://github.com/acquia/developer-portal/pull/45