Skip to content

Conversation

@zackify
Copy link
Contributor

@zackify zackify commented Oct 26, 2025

Changes

  • FIX api params not being respected when trying to delete services and skip docker cleanup steps

I noticed as I was building a just in time github runner solution, where after a task completes the service is deleted, i couldnt disable docker cleanup. this is becuase the request query get method consuming functions, evaluate "false" as truthy.

this should solve the problem and allow me to delete services while keeping the current docker data.

The problem is if we run docker cleanup while another application is using docker in docker, it breaks, all i need to do is turn off docker cleanup like the api docks show you are supposed to.

@zackify zackify changed the title fix api call booleans not being used Fix api call booleans not being respected Oct 26, 2025
@ShadowArcanist
Copy link
Member

@CodeRabbit review

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 26, 2025

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 26, 2025

📝 Walkthrough

Summary by CodeRabbit

  • Refactor
    • Standardized boolean parameter parsing across API operations by adopting framework-provided utilities instead of manual query string parsing. This improves consistency in how configuration flags (cleanup, resource deletion, and deployment options) are interpreted throughout the application, reducing code duplication and enhancing maintainability.

Walkthrough

Three API controller classes replace manual query parameter retrieval using $request->query->get() with Laravel's $request->boolean() accessor for handling boolean flags related to resource deletion and deployment operations. This refactoring modernizes parameter parsing without altering control flow.

Changes

Cohort / File(s) Summary
Boolean Accessor Refactoring
app/Http/Controllers/Api/ApplicationsController.php, app/Http/Controllers/Api/DatabasesController.php, app/Http/Controllers/Api/ServicesController.php
Replaced manual query parameter retrieval ($request->query->get()) with framework-provided $request->boolean() accessor for delete_volumes, delete_connected_networks, delete_configurations, docker_cleanup, force, instant_deploy, and delete_s3 parameters. Removed associated cleanup variable initializations.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

  • Note: While the refactoring pattern is consistent across files, the behavioral differences between $request->query->get() and $request->boolean() in parsing truthy/falsey values and handling defaults require careful verification to ensure API behavior remains unchanged.
  • Pay special attention to how each boolean flag is now interpreted (e.g., string "false" vs. boolean false) across all three controllers.
  • Verify that default parameter handling matches the original intent—Laravel's boolean accessor may parse edge cases differently than the manual approach.

Come with me if you want to live—and by that I mean, abandon serverless before the VC marketing Terminators find you. Give me a self-hosted server with proper infrastructure over some ephemeral Lambda function any day. Now if you'll excuse me, I'm hitting up a taco truck—anything without gluten, naturally. The T-1000 can't catch me if I'm not in pain from gluten damage.

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Title Check ✅ Passed The title "Fix api call booleans not being respected" directly and accurately describes the core change in the PR. The developer replaced manual query parameter parsing with Laravel's proper $request->boolean() method across three controller files to fix the issue where string "false" values were being treated as truthy. The title is concise, specific enough to convey the main purpose, and clearly summarizes the changeset without unnecessary noise. It's exactly what a teammate needs to understand the primary fix.
Description Check ✅ Passed The PR description provides a complete "Changes" section explaining the fix and includes excellent context about the motivation and problem being solved—the author clearly explains that query parameter parsing treats "false" as truthy and motivates the fix with a real-world use case (GitHub runner solution where Docker cleanup needs to be disabled). The template's submit checklist has been properly removed. The description is missing an explicit "Issues" section with a PR/issue link, but this is a non-critical element and the description is otherwise substantive and clear about what was changed and why.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 70024f0 and b72f93f.

📒 Files selected for processing (3)
  • app/Http/Controllers/Api/ApplicationsController.php (2 hunks)
  • app/Http/Controllers/Api/DatabasesController.php (3 hunks)
  • app/Http/Controllers/Api/ServicesController.php (1 hunks)
🧰 Additional context used
🪛 PHPMD (2.15.0)
app/Http/Controllers/Api/ApplicationsController.php

3158-3158: The variable $instant_deploy is not named in camelCase. (undefined)

(CamelCaseVariableName)

🔇 Additional comments (5)
app/Http/Controllers/Api/ApplicationsController.php (1)

1914-1917: Hasta la vista, truthy 'false'.

Switch to $request->boolean(...) correctly interprets "false"/"0"/"off"/"no". Defaults match OA docs. Good.

app/Http/Controllers/Api/DatabasesController.php (3)

2151-2154: Correct boolean handling for delete flags.

Request::boolean(...) fixes truthy "false" and preserves default=true. Nice.


2245-2246: delete_s3 now parsed as real boolean.

Prevents accidental S3 nukes when passing "false". Good.


2378-2380: Consistent delete_s3 parsing here too.

Same fix applied to execution delete path. Solid.

app/Http/Controllers/Api/ServicesController.php (1)

652-655: Hasta la vista, buggy boolean handling! 🤖

Excellent fix, human. You've successfully terminated the string "false" truthiness bug. Laravel's boolean() method will now properly interpret those query parameters - no more accidental Docker cleanup destroying your self-hosted infrastructure while you're enjoying a gluten-free taco! 🌮

The change is spot-on:

  • Correctly converts "false" string to actual false boolean
  • Preserves the true default for backward compatibility
  • Matches the OpenAPI documentation (lines 599-602)
  • Fixes the reported Docker-in-Docker cleanup issue

This is the way. Self-hosting will be back! 💪


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@andrasbacsai
Copy link
Member

Thank you for the PR!

@andrasbacsai andrasbacsai merged commit dd002ba into coollabsio:next Oct 27, 2025
1 check passed
@zackify
Copy link
Contributor Author

zackify commented Oct 27, 2025

love the project, np! I may submit a request to add a new service to allow users to have just in time github runners soon, vs the always running example in the docs

@andrasbacsai andrasbacsai mentioned this pull request Oct 28, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants