Conversation
📝 WalkthroughWalkthroughThis PR replaces a hard-coded runtimes dataset with a Config::getParam('runtimes')-driven mapping, removes the helper getVersions(), and updates frameworks.php to consume templateRuntimes directly. It introduces an environment-driven allow-list (_APP_FUNCTIONS_RUNTIMES) and changes getRuntimes() to accept an allow-list instead of a versions deny-list, propagating that through template runtime registrations. It also consolidates loading of many config files (including runtimes and runtimes-v2) earlier in app/init/configs.php and updates an e2e test to expect a narrowed runtime filter. Estimated code review effort🎯 4 (Complex) | ⏱️ ~45–60 minutes Areas to focus review on:
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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/config/frameworks.php (1)
203-203: Critical: Undefined function call will cause fatal error.Line 203 still calls
getVersions($templateRuntimes['NODE']['versions'], 'node'), but thegetVersionshelper function was removed in this refactor. This will cause a fatal error when the configuration is loaded.Apply this diff to fix the issue:
- 'runtimes' => getVersions($templateRuntimes['NODE']['versions'], 'node'), + 'runtimes' => $templateRuntimes['NODE'],
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
app/config/frameworks.php(14 hunks)app/config/template-runtimes.php(1 hunks)app/config/templates/function.php(41 hunks)app/init/configs.php(1 hunks)
⏰ 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 (6)
app/init/configs.php (1)
7-8: LGTM: Early runtime configuration loading.Moving the runtime configuration loading to the top ensures that runtime data is available when other configuration files (like
template-runtimes.php,frameworks.php, andtemplates/function.php) need to reference it.app/config/template-runtimes.php (1)
5-12: LGTM: Dynamic runtime configuration.The refactoring from hard-coded runtime arrays to dynamically built mappings from
Config::getParam('runtimes')provides better flexibility. The structure—uppercase keys mapping to arrays of "key-version" strings—aligns with the consumption pattern inframeworks.phpandtemplates/function.php.app/config/frameworks.php (1)
17-17: LGTM: Consistent direct runtime references.All frameworks (except tanstack-start on Line 203) correctly use direct references to
$templateRuntimes['NODE']or$templateRuntimes['FLUTTER'], eliminating the need for the removedgetVersionshelper function.Also applies to: 43-43, 69-69, 94-94, 111-111, 136-136, 153-153, 178-178, 228-228, 253-253, 270-270, 286-286, 303-303, 319-319
app/config/templates/function.php (3)
9-21: LGTM: Refactored to use allow-list filtering.The function signature change from a deny-list to an allow-list approach is consistent with the PR objective. The filtering logic correctly uses
in_array($runtime, $allowList)to include only explicitly allowed runtimes.
37-2158: LGTM: Consistent allow-list propagation across all templates.All
getRuntimes()calls throughout the file have been consistently updated to pass the$allowListparameter. This ensures uniform runtime filtering across all function templates.
7-7: Confirm this is intentional—template runtimes behavior when env var is unset differs from other parts of the codebase.The review comment is correct. When
_APP_FUNCTIONS_RUNTIMESis empty or unset, the code in this file filters out all runtimes (sincein_array($runtime, [])always returns false), resulting in zero available runtimes for templates. This behavior is inconsistent with other parts of the codebase:
- Create.php (line 191): Uses
if (!empty($allowList))first, so validation is skipped when empty—allowing all runtimes- This file: Uses
array_filterwithin_arraydirectly—filters out all runtimes when emptyWhile the default in
app/config/variables.phpis'node-16.0,php-8.0,python-3.9,ruby-3.0', this inconsistency should be addressed to ensure predictable behavior and prevent silent failures in edge cases where the env var is unset.Recommend:
- Add
!empty($allowList)guard before applying the filter, or- Use the same ternary pattern as
runtimes-v2.phpfor consistency, or- Document this behavior explicitly
✨ Benchmark results
⚡ Benchmark Comparison
|
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/config/frameworks.php(15 hunks)app/config/templates/function.php(41 hunks)app/init/configs.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/config/frameworks.php
⏰ 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 (4)
app/init/configs.php (1)
10-11: LGTM!Moving runtime configuration loading to the top ensures these are available before downstream configs (like template-runtimes and frameworks) consume them. This prevents duplicate loads and aligns with the new environment-driven runtime provisioning.
app/config/templates/function.php (3)
4-4: LGTM!The System namespace import is required for the
System::getEnvcall on Line 7.
9-20: Function signature change looks good, but depends on fixing the empty allowList issue.The shift from a deny-list (
versionsDenyList) to an allow-list (allowList) approach is a cleaner design. However, this function will return an empty array when$allowListis empty (as flagged in the comment on Line 7), which needs to be addressed.Note: The
in_arraycheck on Line 19 is case-sensitive. Ensure that runtime names in the environment variable exactly match those in$templateRuntimes(including case).
37-2158: LGTM!All calls to
getRuntimesconsistently pass$allowListas the final parameter, aligning with the updated function signature. The usage is uniform across all templates.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/e2e/Services/Functions/FunctionsCustomClientTest.php (1)
436-442: Simplify assertion - logicalOr with single option is redundant.The
logicalOrconstraint with only one option (containsEqual('node-22')) is unnecessarily complex. Since you're now only testing a single runtime, simplify this assertion.Apply this diff to simplify the assertion:
- foreach ($templates['body']['templates'] as $template) { - $this->assertThat( - \array_column($template['runtimes'], 'name'), - $this->logicalOr( - $this->containsEqual('node-22'), - ), - ); - } + foreach ($templates['body']['templates'] as $template) { + $this->assertContains('node-22', \array_column($template['runtimes'], 'name')); + }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/e2e/Services/Functions/FunctionsCustomClientTest.php(2 hunks)
⏰ 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 (Messaging)
- GitHub Check: E2E Service Test (Teams)
- GitHub Check: E2E Service Test (GraphQL)
- GitHub Check: E2E Service Test (Sites)
- GitHub Check: E2E Service Test (Databases/TablesDB)
- GitHub Check: E2E Service Test (Realtime)
- GitHub Check: E2E Service Test (Health)
- GitHub Check: E2E Service Test (Proxy)
- GitHub Check: E2E Service Test (Functions)
- GitHub Check: E2E Service Test (Projects)
- GitHub Check: E2E Service Test (FunctionsSchedule)
- GitHub Check: E2E Service Test (Databases/Legacy)
- 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: Unit Test
- GitHub Check: E2E General Test
- GitHub Check: Benchmark
- GitHub Check: scan
Task: https://linear.app/appwrite/issue/SER-359/function-listtemplates-shows-a-lot-of-runtimes-many-of-which-arent