Fix: Deployment status #9580
Fix: Deployment status #9580
Conversation
WalkthroughThis pull request updates several components of the deployment process. The configuration file now waits longer before taking a screenshot. The router function in the controller has been enhanced with conditions to bypass the deployment status check when an API key indicates it can be ignored. Additionally, the Key class now stores a flag for deployment status and provides a method to access it. Finally, the build deployment process has been refactored to handle screenshot requests concurrently and update status after all operations complete. Changes
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms (20)
🔇 Additional comments (7)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
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: 0
🧹 Nitpick comments (3)
src/Appwrite/Platform/Modules/Functions/Workers/Builds.php (3)
796-800: Consider removing or clarifying ANSI escape code.
Appending the raw ANSI reset code (\x1b[0m) could print unintelligible characters in logs that do not support color escape sequences. Consider either removing it or ensuring log viewers can interpret ANSI codes.
854-924: Leverage concurrency with partial-failure handling.
Usingbatch()for screenshot generation is a good optimization. However, consider handling partial failures more gracefully (for example, capturing any successful screenshots even if one screenshot request fails), as throwing an exception will stop processing of subsequent tasks.
938-950: Provide fallback when screenshots fail.
This change correctly delays the "ready" status until after screenshot generation, but in the catch block above, a failure to generate screenshots simply logs a warning without preventing the deployment from becoming "ready." If a failed screenshot is a concern, consider adding an option to mark the status as partially successful or to retry before finalizing.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
app/config/frameworks.php(1 hunks)app/controllers/general.php(1 hunks)src/Appwrite/Auth/Key.php(4 hunks)src/Appwrite/Platform/Modules/Functions/Workers/Builds.php(6 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
app/controllers/general.php (1)
src/Appwrite/Auth/Key.php (1)
isDeploymentStatusIgnored(83-86)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Setup & Build Appwrite Image
- GitHub Check: scan
🔇 Additional comments (10)
app/config/frameworks.php (1)
141-141: Increased screenshot sleep time for Vue.js frameworkThe sleep time before taking a screenshot for Vue.js applications has been increased from 3000ms to 5000ms. This change allows for more time for the Vue application to fully render before capturing the screenshot, which should improve the reliability of the screenshot process.
src/Appwrite/Auth/Key.php (4)
27-27: Added new deploymentStatusIgnored property to Key classThis property has been added to the constructor parameters with a default value of false. It will allow certain API keys to bypass deployment status checks when accessing resources.
83-86: Added new isDeploymentStatusIgnored methodThis accessor method returns the value of the deploymentStatusIgnored property, allowing other parts of the application to check if an API key should ignore deployment status.
148-148: Added deploymentStatusIgnored to payload extractionThis extracts the deploymentStatusIgnored flag from the JWT payload with a fallback to false if not present, maintaining backward compatibility with existing keys.
166-167: Updated Key constructor call to include deploymentStatusIgnoredThis passes the extracted deploymentStatusIgnored value to the Key constructor, ensuring the property is properly initialized from the JWT payload.
app/controllers/general.php (1)
272-273: Enhanced deployment status check to support status bypassThis change allows API requests with keys that have deploymentStatusIgnored=true to bypass the deployment status check. The addition of the $allowAnyStatus variable provides a clear and readable condition for when to skip the status check.
This enhancement supports the PR objective to improve deployment status functionality by allowing certain API keys to access deployments regardless of their status, which is useful for taking screenshots in both dark and light modes concurrently.
src/Appwrite/Platform/Modules/Functions/Workers/Builds.php (4)
812-812: Docblock addition is fine.
Adding a short docblock comment here for clarity is helpful.
850-850: Re-check the security implications of ignoring deployment status.
Setting'deploymentStatusIgnored' => truecan allow calls to bypass standard deployment-status checks. Verify that this does not introduce security holes or unintended behavior downstream.
1090-1095: Validate exposure of sensitive data in failure logs.
Logging the exception message, file path, and trace is valuable for debugging but can leak details in shared logs. Confirm that these logs are not exposed to unauthorized users or in end-user-facing environments.
44-45:❓ Verification inconclusive
Confirm Swoole extension availability.
The usage ofSwoole\Coroutine\batchintroduces concurrency benefits. Please verify that the production and staging environments have the Swoole extension installed and correctly configured.
🏁 Script executed:
#!/bin/bash # Description: Check if the Swoole extension is loaded. php -m | grep -i swooleLength of output: 25
Action Required: Manually Verify Swoole Extension Installation
The use of
Swoole\Coroutine\batchrelies on having the Swoole extension available. The initial automated check (usingphp -m | grep -i swoole) produced no output, which is inconclusive. Please verify manually that the Swoole extension is installed and correctly configured in both production and staging environments. For example, you might run:
php --ri swoolephp -r "echo extension_loaded('swoole') ? 'Swoole is loaded' : 'Swoole is not loaded';"Manually confirming this will ensure that the concurrency features provided by Swoole function as expected.
✨ Benchmark results
⚡ Benchmark Comparison
|
Docstrings generation was requested by @Meldiron. * #9580 (comment) The following files were modified: * `app/controllers/general.php` * `src/Appwrite/Auth/Key.php` * `src/Appwrite/Platform/Modules/Functions/Workers/Builds.php` * `src/Appwrite/Transformation/Adapter/Preview.php`
|
Note Generated docstrings for this pull request at #9589 |
What does this PR do?
Test Plan
Cant find any easy way to add tests for this. Manual QA and existing tests is good enough IMO
Related PRs and Issues
x
Checklist
Summary by CodeRabbit
New Features
Bug Fixes
Refactor