CLI-1696: MEO commands #1955
CLI-1696: MEO commands #1955
Conversation
* APPCODE-1994: Support translation endpoints * prerelease
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1955 +/- ##
============================================
- Coverage 92.28% 92.27% -0.01%
- Complexity 1910 1916 +6
============================================
Files 122 122
Lines 6985 6993 +8
============================================
+ Hits 6446 6453 +7
- Misses 539 540 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Try the dev build for this PR: https://acquia-cli.s3.amazonaws.com/build/pr/1955/acli.phar |
There was a problem hiding this comment.
Pull request overview
This PR adds support for MEO (Managed Edge Orchestration) translation API commands to the Acquia CLI. It introduces three new API endpoints for managing environment logs and code switching operations, along with response transformation logic to strip _links from translation API responses.
Changes:
- Added three new translation API endpoints: log listing, log downloading, and code switching
- Implemented response munging logic to remove
_linksproperties from translation API responses - Added test coverage for the translation response transformation
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| vfs:/root/.config/composer/.htaccess | Test artifact accidentally committed - should be removed |
| tests/phpunit/src/Commands/Api/ApiCommandTest.php | Added test case to verify _links removal from translation API responses |
| src/Command/Api/ApiBaseCommand.php | Implemented mungeResponse() method to strip _links from translation endpoint responses |
| assets/acquia-spec.json | Added OpenAPI specifications for three new translation endpoints (log-list, log-download, code-switch) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| foreach ($response as $value) { | ||
| if (property_exists($value, '_links')) { | ||
| unset($value->_links); | ||
| } | ||
| } |
There was a problem hiding this comment.
The foreach loop assumes that $response is iterable, but this may not always be the case. If $response is an object (but not iterable) or a scalar value, this will throw a TypeError. Consider adding a check to ensure $response is iterable before attempting to foreach over it, or use is_array() or is_iterable() guards.
| unset($response->_links); | ||
| } | ||
| foreach ($response as $value) { | ||
| if (property_exists($value, '_links')) { |
There was a problem hiding this comment.
The mungeResponse() method doesn't check if $value is an object before calling property_exists(). If $value is a scalar type (string, int, etc.), property_exists() will trigger a warning. Add a type check like is_object($value) before calling property_exists().
| if (property_exists($value, '_links')) { | |
| if (is_object($value) && property_exists($value, '_links')) { |
| private function mungeResponse(mixed $response): void | ||
| { | ||
| if (is_object($response) && property_exists($response, '_links')) { | ||
| unset($response->_links); | ||
| } | ||
| foreach ($response as $value) { | ||
| if (property_exists($value, '_links')) { | ||
| unset($value->_links); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The mungeResponse() method lacks documentation explaining its purpose and behavior. Add a docblock that explains why this method is needed specifically for translation endpoints and what transformations it performs on the response.
| "items": { | ||
| "type": "array", | ||
| "example": [ | ||
| { | ||
| "name": "apache-access", | ||
| "type": "apache-access", | ||
| "label": "Apache access", | ||
| "flags": { | ||
| "available": false | ||
| }, | ||
| "_links": { | ||
| "download": { | ||
| "href": "https://cloud.acquia.com/api/translation/environments/830ea829-490a-4e2e-a16b-ff055fc58a0e/logs/apache-access" | ||
| } | ||
| } | ||
| }, | ||
| { | ||
| "name": "apache-error", | ||
| "type": "apache-error", | ||
| "label": "Apache error", | ||
| "flags": { | ||
| "available": false | ||
| }, | ||
| "_links": { | ||
| "download": { | ||
| "href": "https://cloud.acquia.com/api/translation/environments/830ea829-490a-4e2e-a16b-ff055fc58a0e/logs/apache-error" | ||
| } | ||
| } | ||
| }, | ||
| { | ||
| "name": "cronjob", | ||
| "type": "cronjob", | ||
| "label": "Cronjob", | ||
| "flags": { | ||
| "available": false | ||
| }, | ||
| "_links": { | ||
| "download": { | ||
| "href": "https://cloud.acquia.com/api/translation/environments/830ea829-490a-4e2e-a16b-ff055fc58a0e/logs/cronjob" | ||
| } | ||
| } | ||
| } | ||
| ], | ||
| "items": { | ||
| "type": "object", | ||
| "properties": { | ||
| "name": { | ||
| "type": "string" | ||
| }, | ||
| "type": { | ||
| "type": "string" | ||
| }, | ||
| "label": { | ||
| "type": "string" | ||
| }, | ||
| "flags": { | ||
| "type": "object", | ||
| "properties": { | ||
| "available": { | ||
| "type": "boolean" | ||
| } | ||
| } | ||
| }, | ||
| "_links": { | ||
| "type": "object", | ||
| "properties": { | ||
| "download": { | ||
| "type": "object", | ||
| "properties": { | ||
| "href": { | ||
| "type": "string", | ||
| "format": "string" | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
There is a duplicate "items" key in the JSON schema. Line 46770 defines "items" with "type": "array" and an example, while line 46812 defines "items" again with "type": "object" and properties. In JSON, duplicate keys are not allowed and the second occurrence will overwrite the first. The schema structure should be corrected to avoid this duplication.
| $exitCode = 1; | ||
| } | ||
|
|
||
| if (substr($this->path, 0, 12) === '/translation') { |
There was a problem hiding this comment.
The hard-coded path prefix check using substr() is fragile and could become a maintenance burden as more translation endpoints are added. Consider using a more robust approach such as str_starts_with() (available in PHP 8.0+) or checking if the path matches a pattern. Additionally, consider whether this logic should be driven by configuration or metadata from the API spec rather than being hard-coded.
| if (substr($this->path, 0, 12) === '/translation') { | |
| if (str_starts_with($this->path, '/translation')) { |
1 participant
Motivation
Fixes CLI-1696
Proposed changes
Alternatives considered
Testing steps
./bin/acli ckc