Feat: Custom start commands#10842
Conversation
📝 WalkthroughWalkthroughThis pull request introduces support for custom start commands across Functions and Sites deployments. The feature adds a new Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
Security Scan Results for PRDocker Image Scan Results
Source Code Scan Results🎉 No vulnerabilities found! |
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/controllers/api/vcs.php (1)
1781-1829:createGitDeploymentscall uses outdated signature (runtime fatal)The
$createGitDeploymentsclosure now expects many more parameters (providerBranchUrl, repo metadata, commit details, etc.), but this call still passes only the older subset:$createGitDeployments($github, $providerInstallationId, $repositories, $providerBranch, $providerCommitHash, $providerPullRequestId, true, $dbForPlatform, $queueForBuilds, $getProjectDB, $request);This will throw an argument count error as soon as this endpoint is exercised.
You need to either (a) extend this endpoint to derive the additional metadata from
$pullRequestResponse(similar to the webhookEVENT_PULL_REQUESTpath above) and pass them through, or (b) refactorcreateGitDeploymentsso the new parameters are optional/defaulted and compatible with this call site.A sketch using the same pattern as the webhook handler:
- $providerBranch = \explode(':', $pullRequestResponse['head']['label'])[1] ?? ''; - $providerCommitHash = $pullRequestResponse['head']['sha'] ?? ''; - - $createGitDeployments($github, $providerInstallationId, $repositories, $providerBranch, $providerCommitHash, $providerPullRequestId, true, $dbForPlatform, $queueForBuilds, $getProjectDB, $request); + $providerBranch = \explode(':', $pullRequestResponse['head']['label'])[1] ?? ''; + $providerCommitHash = $pullRequestResponse['head']['sha'] ?? ''; + + // Derive these similarly to the webhook EVENT_PULL_REQUEST logic above: + $providerRepositoryName = /* from $pullRequestResponse or $repositoryName */; + $providerRepositoryOwner = /* from $pullRequestResponse or $owner */; + $providerRepositoryUrl = /* https://github.com/$providerRepositoryOwner/$providerRepositoryName */; + $providerBranchUrl = /* $providerRepositoryUrl . '/tree/' . $providerBranch */; + $providerCommitAuthor = /* from $github->getCommit(...) if needed */; + $providerCommitAuthorUrl = /* from commit details or payload */; + $providerCommitMessage = /* from commit details or payload */; + $providerCommitUrl = /* from commit details or payload */; + + $createGitDeployments( + $github, + $providerInstallationId, + $repositories, + $providerBranch, + $providerBranchUrl, + $providerRepositoryName, + $providerRepositoryUrl, + $providerRepositoryOwner, + $providerCommitHash, + $providerCommitAuthor, + $providerCommitAuthorUrl, + $providerCommitMessage, + $providerCommitUrl, + $providerPullRequestId, + true, + $dbForPlatform, + $queueForBuilds, + $getProjectDB, + $request + );Adjust the
/* ... */placeholders to match however your GitHub adapter structures the PR and commit data.
🧹 Nitpick comments (2)
src/Appwrite/Platform/Modules/Functions/Http/Executions/Create.php (1)
398-402: Deployment-levelstartCommandoverride is wired correctlyUsing the runtime’s
startCommandas a default and overriding withdeployment.startCommand(with acd /usr/local/server/src/function/ &&prefix) is consistent and keeps backward compatibility; only minor nit is the duplicatedgetAttributecall, which could be hoisted into a local variable if you ever touch this again.tests/resources/sites/astro-custom-start-command/custom-server.js (1)
12-14: Misleading catch-all error handler.The catch-all middleware returns a 500 status code for all unmatched requests, which typically indicates a server error. However, this handler will also catch legitimate 404 (Not Found) cases. For a test fixture, this might be intentional, but it's worth noting that a more accurate implementation would use a 404 status code for unmatched routes.
If this behavior is intentional for testing error handling, consider adding a comment. Otherwise, consider this alternative:
app.use((_req, res) => { - res.status(500).send('Custom error'); + res.status(404).send('Not found'); });
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
tests/resources/sites/astro-custom-start-command/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (24)
app/config/collections/projects.php(3 hunks)app/controllers/api/vcs.php(1 hunks)app/controllers/general.php(1 hunks)src/Appwrite/Platform/Modules/Compute/Base.php(2 hunks)src/Appwrite/Platform/Modules/Functions/Http/Deployments/Create.php(2 hunks)src/Appwrite/Platform/Modules/Functions/Http/Deployments/Duplicate/Create.php(1 hunks)src/Appwrite/Platform/Modules/Functions/Http/Deployments/Template/Create.php(1 hunks)src/Appwrite/Platform/Modules/Functions/Http/Executions/Create.php(1 hunks)src/Appwrite/Platform/Modules/Functions/Http/Functions/Create.php(1 hunks)src/Appwrite/Platform/Modules/Sites/Http/Deployments/Create.php(2 hunks)src/Appwrite/Platform/Modules/Sites/Http/Deployments/Duplicate/Create.php(1 hunks)src/Appwrite/Platform/Modules/Sites/Http/Deployments/Template/Create.php(1 hunks)src/Appwrite/Platform/Modules/Sites/Http/Sites/Create.php(3 hunks)src/Appwrite/Platform/Modules/Sites/Http/Sites/Update.php(3 hunks)src/Appwrite/Platform/Workers/Functions.php(1 hunks)src/Appwrite/Utopia/Response/Model/Func.php(1 hunks)src/Appwrite/Utopia/Response/Model/Site.php(1 hunks)tests/e2e/Services/Sites/SitesCustomServerTest.php(1 hunks)tests/resources/sites/astro-custom-start-command/.gitignore(1 hunks)tests/resources/sites/astro-custom-start-command/astro.config.mjs(1 hunks)tests/resources/sites/astro-custom-start-command/custom-server.js(1 hunks)tests/resources/sites/astro-custom-start-command/package.json(1 hunks)tests/resources/sites/astro-custom-start-command/src/pages/index.astro(1 hunks)tests/resources/sites/astro-custom-start-command/src/pages/ssr.js(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
src/Appwrite/Utopia/Response/Model/Func.php (1)
src/Appwrite/Utopia/Response/Model.php (1)
addRule(90-102)
src/Appwrite/Platform/Modules/Sites/Http/Sites/Update.php (1)
src/Appwrite/GraphQL/Types/Mapper.php (1)
param(255-363)
tests/e2e/Services/Sites/SitesCustomServerTest.php (2)
tests/e2e/Services/Sites/SitesBase.php (6)
setupSite(21-34)setupSiteDomain(369-387)setupDeployment(36-83)packageSite(239-251)getSiteDomain(389-410)cleanupSite(85-94)tests/e2e/Client.php (2)
Client(8-342)setEndpoint(140-145)
src/Appwrite/Utopia/Response/Model/Site.php (1)
src/Appwrite/Utopia/Response/Model.php (1)
addRule(90-102)
src/Appwrite/Platform/Modules/Sites/Http/Sites/Create.php (1)
src/Appwrite/GraphQL/Types/Mapper.php (1)
param(255-363)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Setup & Build Appwrite Image
- GitHub Check: Setup & Build Appwrite Image
- GitHub Check: scan
🔇 Additional comments (19)
tests/resources/sites/astro-custom-start-command/.gitignore (1)
1-24: .gitignore patterns are appropriate for the Astro test fixtureCovers build output, dependencies, logs, env files, and local OS/IDE artifacts; nothing concerning here.
app/config/collections/projects.php (3)
768-779: Function collection gainsstartCommandin a schema-consistent wayThe
functions.startCommandattribute mirrors the nearbyentrypoint/commandsfields (string, large size, optional, scalar). The comment noting it’s currently unused by runtimes is helpful and harmless.
1050-1060: Site collectionstartCommandfield matches existing command fieldsDefining
sites.startCommandas an optional long string right afterinstallCommand/buildCommandkeeps the schema coherent and enables site-level start command configuration without impacting existing data.
1383-1393: Deployment-levelstartCommandfield is aligned with build metadataAdding
deployments.startCommandnext tobuildCommands/buildOutputis consistent with how deployment configuration is stored, and matches how other modules now readstartCommandfrom deployments.app/controllers/api/vcs.php (1)
295-309: PersistingstartCommandinto VCS-created deployments looks correctThreading
$resource->getAttribute('startCommand', '')into the deployments created from VCS repositories keeps these flows consistent with other deployment-creation paths that now carrystartCommand.src/Appwrite/Platform/Modules/Sites/Http/Deployments/Duplicate/Create.php (1)
104-134: Duplicate site deployments correctly carry overstartCommandUsing
$site->getAttribute('startCommand', '')when creating the duplicate deployment keeps custom start commands in sync with current site configuration, just likeoutputDirectoryandadapter.src/Appwrite/Platform/Modules/Functions/Http/Deployments/Duplicate/Create.php (1)
101-115: Function duplicate deployments now includestartCommandPopulating
startCommandfrom the function when duplicating a deployment aligns with howentrypointandcommandsare taken, so any future function-level custom start commands won’t be lost on rebuild.src/Appwrite/Utopia/Response/Model/Site.php (1)
128-133: API model exposesstartCommandwith sensible defaultsThe new
startCommandrule (string, empty default, example command) fits cleanly alongsideinstallCommand/buildCommandand makes the feature visible in the public Site model without changing existing behavior.src/Appwrite/Platform/Modules/Compute/Base.php (2)
97-128: VCS function redeploys preservestartCommandIncluding
'startCommand' => $function->getAttribute('startCommand', '')in the deployment document ensures redeploys triggered via VCS keep any configured start command in sync with the function.
195-228: VCS site redeploys also persiststartCommandAdding
startCommandalongsidebuildCommands/buildOutputfor site deployments keeps the VCS redeploy path aligned with the rest of the site deployment stack and with the new schema fields.tests/resources/sites/astro-custom-start-command/src/pages/index.astro (1)
1-16: LGTM! Simple test resource page.This is a straightforward Astro page serving as a test fixture for the custom start command feature. The minimal structure is appropriate for testing purposes.
tests/resources/sites/astro-custom-start-command/astro.config.mjs (1)
1-10: LGTM! Appropriate Astro configuration for custom server test.This Astro configuration correctly sets up the Node adapter in middleware mode, which is necessary for the custom Express server implementation in
custom-server.js. The middleware mode allows the SSR handler to be mounted as Express middleware, enabling the custom routes and error handling in the test fixture.src/Appwrite/Platform/Modules/Sites/Http/Deployments/Template/Create.php (1)
166-166: Code is correct — startCommand is properly populated during site lifecycle.Verification confirms that the
startCommandattribute is appropriately initialized and maintained:
- Site creation (Sites/Create.php line 155) stores the user-provided
startCommand- Site updates (Sites/Update.php line 259) properly update the field
- Line 166 in the reviewed file correctly reads the attribute with an empty string fallback
The implementation follows the established pattern across all deployment creation flows and safely handles the optional nature of this field.
tests/resources/sites/astro-custom-start-command/src/pages/ssr.js (1)
1-5: SSR route implementation matches Astro patterns and test needs
prerender = falseand theGEThandler returning a dynamic"SSR OK (timestamp)"response look correct and align with the e2e test assertions.tests/resources/sites/astro-custom-start-command/package.json (1)
1-18: Package definition is consistent with the Astro custom start-command fixtureScripts and dependencies match the e2e usage (build, copy
custom-server.js, and produce a production-only install). No issues spotted for this test resource.src/Appwrite/Platform/Modules/Functions/Http/Deployments/Template/Create.php (1)
145-165: PropagatingstartCommandinto template deployments is consistentIncluding:
'startCommand' => $function->getAttribute('startCommand', ''),in the deployment document keeps template deployments aligned with the function’s configured start command and mirrors other deployment flows. No additional changes needed here.
src/Appwrite/Platform/Modules/Functions/Http/Deployments/Create.php (1)
233-253:startCommandwiring for function deployments covers both full and chunked uploadsAdding:
'startCommand' => $function->getAttribute('startCommand', ''),to both the completed-upload and initial-chunk deployment creation blocks ensures every new deployment consistently snapshots the function’s start command. This matches how
buildCommandsis handled and looks correct.Also applies to: 272-293
src/Appwrite/Platform/Modules/Sites/Http/Deployments/Create.php (1)
243-265: Site deployments now correctly snapshotstartCommandThe added lines:
'startCommand' => $site->getAttribute('startCommand', ''),in both deployment creation branches ensure each deployment carries the site’s configured start command, regardless of whether the upload is single-shot or chunked. This is consistent with how build commands and adapter metadata are stored.
Also applies to: 310-333
src/Appwrite/Platform/Modules/Sites/Http/Sites/Create.php (1)
72-72:startCommandis correctly threaded through site creationThe new
startCommandparameter:->param('startCommand', '', new Text(8192, 0), 'Custom start command. Leave empty to use default.', true)and its use in
action()and the createdsitesdocument:'startCommand' => $startCommand,cleanly extend the site creation API and persist the value expected by the deployments module. This is consistent with the update and deployment flows.
Also applies to: 105-105, 143-170
✨ Benchmark results
⚡ Benchmark Comparison
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/e2e/Services/Sites/SitesCustomServerTest.php (1)
2960-2969: Remove redundant domain assignment.Line 2960 assigns
$domainfromsetupSiteDomain(), but line 2969 immediately overwrites it withgetSiteDomain(). While thesetupSiteDomain()call is needed for its side effect (creating the domain rule), the assignment to$domainis unnecessary.Apply this diff:
- $domain = $this->setupSiteDomain($siteId); + $this->setupSiteDomain($siteId); $deploymentId = $this->setupDeployment($siteId, [ 'code' => $this->packageSite('astro-custom-start-command'),
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/Appwrite/Platform/Modules/Sites/Http/Sites/Update.php(4 hunks)src/Appwrite/Utopia/Response/Model/Func.php(1 hunks)tests/e2e/Services/Sites/SitesCustomServerTest.php(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
tests/e2e/Services/Sites/SitesCustomServerTest.php (2)
tests/e2e/Services/Sites/SitesBase.php (6)
setupSite(21-34)setupSiteDomain(369-387)setupDeployment(36-83)packageSite(239-251)getSiteDomain(389-410)cleanupSite(85-94)tests/e2e/Client.php (2)
Client(8-342)setEndpoint(140-145)
src/Appwrite/Utopia/Response/Model/Func.php (1)
src/Appwrite/Utopia/Response/Model.php (1)
addRule(90-102)
src/Appwrite/Platform/Modules/Sites/Http/Sites/Update.php (1)
src/Appwrite/GraphQL/Types/Mapper.php (1)
param(255-363)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: E2E Service Test (Webhooks)
- GitHub Check: E2E Service Test (Sites)
- GitHub Check: E2E Service Test (Messaging)
- GitHub Check: E2E Service Test (Teams)
- GitHub Check: E2E Service Test (Users)
- GitHub Check: E2E Service Test (Projects)
- GitHub Check: E2E Service Test (Tokens)
- GitHub Check: E2E Service Test (Storage)
- GitHub Check: E2E Service Test (Proxy)
- GitHub Check: E2E Service Test (Locale)
- GitHub Check: E2E Service Test (Health)
- GitHub Check: E2E Service Test (Functions)
- GitHub Check: E2E Service Test (Realtime)
- GitHub Check: E2E Service Test (Console)
- GitHub Check: E2E Service Test (Account)
- GitHub Check: E2E Service Test (Site Screenshots)
- GitHub Check: E2E Service Test (Dev Keys)
- GitHub Check: E2E General Test
- GitHub Check: Benchmark
- GitHub Check: scan
🔇 Additional comments (2)
src/Appwrite/Utopia/Response/Model/Func.php (1)
143-148: LGTM! The description has been corrected.The
startCommandrule is properly implemented and the description now correctly references "function runtime" instead of "site runtime". The field follows the same pattern as other command-related fields in the model.src/Appwrite/Platform/Modules/Sites/Http/Sites/Update.php (1)
76-76: LGTM! ThestartCommandparameter is fully integrated.The
startCommandfield has been properly added to:
- API parameter definition (line 76)
- Action method signature (line 113)
- Live state change detection (line 231)
- Document persistence (line 260)
The past review concern about including
startCommandin the live change detection has been addressed. Changes to the start command will now correctly mark the site as not live, requiring a redeploy.Also applies to: 113-113, 231-231, 260-260
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
src/Appwrite/Platform/Modules/Functions/Http/Functions/Create.php (1)
351-351:startCommandattribute is never set on function documents.This issue was already identified in a previous review. The function creation logic (lines 213-243) does not include a
startCommandfield, and the function creation endpoint does not accept it as a parameter (lines 79-111). Therefore,$function->getAttribute('startCommand', '')will always return an empty string.As noted in the previous review, either add
startCommandas an optional parameter to the function create/update endpoints, or remove this line if the feature is not intended for Functions.tests/e2e/Services/Sites/SitesCustomServerTest.php (1)
2987-2996: Remove redundant domain assignment.Line 2987 calls
setupSiteDomain($siteId)and assigns to$domain, but line 2996 immediately overwrites it withgetSiteDomain($siteId). The first assignment is unnecessary.Apply this diff:
- $domain = $this->setupSiteDomain($siteId); + $this->setupSiteDomain($siteId); $deploymentId = $this->setupDeployment($siteId, [ 'code' => $this->packageSite('astro-custom-start-command'), 'activate' => 'true' ]); $this->assertNotEmpty($deploymentId); $domain = $this->getSiteDomain($siteId);
🧹 Nitpick comments (1)
tests/e2e/Services/Sites/SitesCustomServerTest.php (1)
3007-3011: Consider more robust assertions for SSR validation.The test validates server-side rendering by comparing two responses and asserting they differ (due to
Date.now()). While this works, it could theoretically fail if both requests complete within the same millisecond.As an alternative, you could assert the presence of expected substrings like
"SSR OK ("rather than relying on inequality, which would be more deterministic while still confirming SSR functionality.Also applies to: 3016-3020
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
app/config/collections/projects.php(3 hunks)app/controllers/general.php(1 hunks)src/Appwrite/Platform/Modules/Functions/Http/Executions/Create.php(1 hunks)src/Appwrite/Platform/Modules/Functions/Http/Functions/Create.php(1 hunks)src/Appwrite/Platform/Modules/Sites/Http/Sites/Create.php(3 hunks)src/Appwrite/Platform/Modules/Sites/Http/Sites/Update.php(4 hunks)src/Appwrite/Platform/Workers/Functions.php(1 hunks)src/Appwrite/Utopia/Response/Model/Site.php(1 hunks)tests/e2e/Services/Sites/SitesCustomServerTest.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- src/Appwrite/Platform/Modules/Functions/Http/Executions/Create.php
- app/controllers/general.php
- src/Appwrite/Platform/Workers/Functions.php
- src/Appwrite/Platform/Modules/Sites/Http/Sites/Create.php
- src/Appwrite/Platform/Modules/Sites/Http/Sites/Update.php
- app/config/collections/projects.php
🧰 Additional context used
🧬 Code graph analysis (2)
src/Appwrite/Utopia/Response/Model/Site.php (1)
src/Appwrite/Utopia/Response/Model.php (1)
addRule(90-102)
tests/e2e/Services/Sites/SitesCustomServerTest.php (2)
tests/e2e/Services/Sites/SitesBase.php (6)
setupSite(21-34)setupSiteDomain(369-387)setupDeployment(36-83)packageSite(239-251)getSiteDomain(389-410)cleanupSite(85-94)tests/e2e/Client.php (2)
Client(8-342)setEndpoint(140-145)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Benchmark
- GitHub Check: Setup & Build Appwrite Image
- GitHub Check: scan
🔇 Additional comments (2)
src/Appwrite/Utopia/Response/Model/Site.php (1)
128-133: LGTM! Clean addition following existing patterns.The
startCommandfield is well-structured and consistent with sibling fields (installCommand,buildCommand). Verification confirms input validation is in place at the controller/API layer using theText(8192, 0)validator, identical to the existing command fields.tests/e2e/Services/Sites/SitesCustomServerTest.php (1)
2970-2970: Address the question about function tests.A previous reviewer asked whether tests for functions should be added. Since this PR adds custom start command support for sites (and possibly functions), but this test file is specifically
SitesCustomServerTest, a separate test for functions would belong in a different test file (e.g.,FunctionsCustomServerTestor similar).If custom start commands are also supported for functions in this PR, please confirm that corresponding function tests exist in the appropriate test file.
What does this PR do?
Adds custom start commands, for sites only right now
Test Plan
New tests
Related PRs and Issues
x
Checklist