feat: add support for faster resource pulls without conflicts by raghavyuva · Pull Request #799 · raghavyuva/nixopus

raghavyuva · 2025-12-25T06:59:05Z

User description

Issue

Link to related issue(s):

Description

Short summary of what this PR changes or introduces.

Scope of Change

Select all applicable areas impacted by this PR:

Screenshot / Video / GIF (if applicable)

Attach or embed screenshots, screen recordings, or GIFs demonstrating the feature or fix.

Related PRs (if any)

Link any related or dependent PRs across repos.

Additional Notes for Reviewers (optional)

Anything reviewers should know before testing or merging (e.g., environment variables, setup steps).

Developer Checklist

To be completed by the developer who raised the PR.

Add valid/relevant title for the PR
Self-review done
Manual dev testing done
No secrets exposed
No merge conflicts
Docs added/updated (if applicable)
Removed debug prints / secrets / sensitive data
Unit / Integration tests passing
Follows all standards defined in Nixopus Docs

Reviewer Checklist

To be completed by the reviewer before merge.

Peer review done
No console.logs / fmt.prints left
No secrets exposed
If any DB migrations, migration changes are verified
Verified release changes are production-ready

PR Type

Enhancement

Description

Disable repository persistence by always removing cloned repos after builds
Add CleanupRepository() method to remove repositories post-deployment
Implement fresh clones instead of pull operations for consistency
Add CloneWithBranch() for efficient shallow clones of specific branches
Simplify GetClonePath() by removing pull logic and return value

Diagram Walkthrough

flowchart LR
  A["Clone Repository"] --> B["Build Image"]
  B --> C["Cleanup Repository"]
  C --> D["Deploy Container"]
  E["Build Failure"] --> C
  F["Fresh Clone Strategy"] --> A
  G["Shallow Clone with Branch"] --> A

File Walkthrough

Relevant files

Enhancement

clone.go `Add repository cleanup functionality` api/internal/features/deploy/tasks/clone.go Add logger import for cleanup operations Implement new `CleanupRepository()` method to remove cloned repos Method logs warnings on cleanup failures and info on success	+14/-0
create.go `Integrate cleanup into create deployment flow` api/internal/features/deploy/tasks/create.go Call `CleanupRepository()` after successful image build Call `CleanupRepository()` on build failure for cleanup Ensures repositories are removed regardless of deployment outcome	+5/-0
redeploy.go `Integrate cleanup into redeploy flow` api/internal/features/deploy/tasks/redeploy.go Call `CleanupRepository()` after successful image build Call `CleanupRepository()` on build failure Maintains consistent cleanup pattern across deployment types	+5/-0
update.go `Integrate cleanup into update deployment flow` api/internal/features/deploy/tasks/update.go Call `CleanupRepository()` after successful image build Call `CleanupRepository()` on build failure Ensures cleanup consistency in update deployment workflow	+5/-0
authenticated_url.go `Simplify clone path logic for fresh clones` api/internal/features/github-connector/service/authenticated_url.go Update `GetClonePath()` documentation to reflect fresh clone strategy Remove `shouldPull` return value and logic Always create fresh directories instead of checking for existing repos Simplify function signature from returning `(string, bool, error)` to `(string, error)`	+10/-16
clone_repository.go `Replace pull strategy with fresh clone approach` api/internal/features/github-connector/service/clone_repository.go Remove `should_pull` variable from `GetClonePath()` call Always remove existing repository before cloning for fresh state Add explicit rollback handling with full clone and branch switching Implement branch-specific shallow clones for non-rollback deployments Remove pull-based update logic in favor of fresh clones	+38/-15
git_client.go `Add shallow clone with branch support` api/internal/features/github-connector/service/git_client.go Add `CloneWithBranch()` method to interface for shallow branch clones Implement `CloneWithBranch()` using `--depth=1 --branch` flags Enables efficient cloning of specific branches without full history	+21/-0

coderabbitai · 2025-12-25T06:59:11Z

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

✨ Finishing touches

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment
Commit unit tests in branch feat/disable_deployment_code_persistance

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.}

…yment_code_persistance

qodo-code-review · 2025-12-25T06:59:37Z

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
⚪	Command injection Description: Remote command injection risk: `CloneWithBranch()` builds an SSH shell command via `fmt.Sprintf("git clone --depth=1 --branch=%s %s %s", branch, repoURL, destinationPath)` using unquoted/unvalidated inputs (`branch`, and potentially `repoURL`/`destinationPath`), enabling injection such as a crafted branch name like `main; rm -rf /` to execute arbitrary commands on the remote host. git_client.go [63-76] Referred Code func (g *DefaultGitClient) CloneWithBranch(repoURL, destinationPath, branch string) error { client, err := g.ssh.Connect() if err != nil { return fmt.Errorf("failed to connect via SSH: %w", err) } defer client.Close() // Use shallow clone (--depth=1) and clone only the specific branch (--branch) // This is more efficient as it only downloads the necessary branch cmd := fmt.Sprintf("git clone --depth=1 --branch=%s %s %s", branch, repoURL, destinationPath) output, err := client.Run(cmd) if err != nil { return fmt.Errorf("git clone with branch failed: %s, output: %s", err.Error(), output) }
Ticket Compliance
⚪	🎫 No ticket provided Create ticket/issue
Codebase Duplication Compliance
⚪	Codebase context is not defined Follow the guide to enable codebase context checks.
Custom Compliance
🟢	Generic: Meaningful Naming and Self-Documenting Code Objective: Ensure all identifiers clearly express their purpose and intent, making code self-documenting Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Robust Error Handling and Edge Case Management Objective: Ensure comprehensive error handling that provides meaningful context and graceful degradation Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
🔴	Generic: Comprehensive Audit Trails Objective: To create a detailed and reliable record of critical system actions for security analysis and compliance. Status: Missing actor context: Repository deletion/cleanup is logged without a user/actor identifier (empty string passed), reducing auditability of this critical action. Referred Code func (t *TaskService) CleanupRepository(repoPath string) { if repoPath == "" { return } if err := t.Github_service.RemoveRepository(repoPath); err != nil { t.Logger.Log(logger.Warning, fmt.Sprintf("Failed to cleanup repository at %s: %s", repoPath, err.Error()), "") } else { t.Logger.Log(logger.Info, fmt.Sprintf("Successfully cleaned up repository at %s", repoPath), "") } Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Security-First Input Validation and Data Handling Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent vulnerabilities Status: Command injection risk: The `branch` value is interpolated directly into a remote shell command without validation/escaping, enabling command injection if `branch` is attacker-controlled. Referred Code // CloneWithBranch clones a git repository with a shallow clone (depth=1) of only the specified branch func (g *DefaultGitClient) CloneWithBranch(repoURL, destinationPath, branch string) error { client, err := g.ssh.Connect() if err != nil { return fmt.Errorf("failed to connect via SSH: %w", err) } defer client.Close() // Use shallow clone (--depth=1) and clone only the specific branch (--branch) // This is more efficient as it only downloads the necessary branch cmd := fmt.Sprintf("git clone --depth=1 --branch=%s %s %s", branch, repoURL, destinationPath) output, err := client.Run(cmd) if err != nil { return fmt.Errorf("git clone with branch failed: %s, output: %s", err.Error(), output) } Learn more about managing compliance generic rules or creating your own custom rules
⚪	Generic: Secure Error Handling Objective: To prevent the leakage of sensitive system information through error messages while providing sufficient detail for internal debugging. Status: Error output exposure: The returned error includes raw command output which may reveal internal paths/host details if propagated to user-facing surfaces. Referred Code // CloneWithBranch clones a git repository with a shallow clone (depth=1) of only the specified branch func (g *DefaultGitClient) CloneWithBranch(repoURL, destinationPath, branch string) error { client, err := g.ssh.Connect() if err != nil { return fmt.Errorf("failed to connect via SSH: %w", err) } defer client.Close() // Use shallow clone (--depth=1) and clone only the specific branch (--branch) // This is more efficient as it only downloads the necessary branch cmd := fmt.Sprintf("git clone --depth=1 --branch=%s %s %s", branch, repoURL, destinationPath) output, err := client.Run(cmd) if err != nil { return fmt.Errorf("git clone with branch failed: %s, output: %s", err.Error(), output) } g.logger.Log(logger.Info, fmt.Sprintf("Successfully cloned branch %s of repository to %s", branch, destinationPath), "") return nil Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Secure Logging Practices Objective: To ensure logs are useful for debugging and auditing without exposing sensitive information like PII, PHI, or cardholder data. Status: Unstructured path logs: Cleanup logs embed full `repoPath` in free-form strings, which may leak identifiable filesystem layout/user identifiers depending on what `repoPath` contains and how logs are stored. Referred Code if err := t.Github_service.RemoveRepository(repoPath); err != nil { t.Logger.Log(logger.Warning, fmt.Sprintf("Failed to cleanup repository at %s: %s", repoPath, err.Error()), "") } else { t.Logger.Log(logger.Info, fmt.Sprintf("Successfully cleaned up repository at %s", repoPath), "") } Learn more about managing compliance generic rules or creating your own custom rules
Update

Compliance status legend

🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

qodo-code-review · 2025-12-25T07:00:47Z

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Fix undefined variable and missing logic Fix a compilation error by reintroducing the missing image-building logic that defines `buildImageResult`. Also, correct the misleading error message for the cloning step. api/internal/features/deploy/tasks/create.go [62-70] if err != nil { - taskCtx.LogAndUpdateStatus("Failed to build image: "+err.Error(), shared_types.Failed) - // Cleanup repository even on build failure + taskCtx.LogAndUpdateStatus("Failed to clone repository: "+err.Error(), shared_types.Failed) + // Cleanup repository even on clone failure t.CleanupRepository(repoPath) return err } - taskCtx.AddLog("Image built successfully: " + buildImageResult + " for application " + TaskPayload.Application.Name) + // TODO: Re-introduce image build step here. + // For example: + // buildImageResult, err := t.BuildImage(repoPath, ...) + // if err != nil { ... } + + taskCtx.AddLog("Repository cloned successfully for application " + TaskPayload.Application.Name) taskCtx.UpdateStatus(shared_types.Deploying) Apply / Chat Suggestion importance[1-10]: 10 __ Why: This suggestion correctly identifies a critical compilation error where `buildImageResult` is used but no longer defined due to the refactoring. It also points out a misleading error message, and that the image building logic is now missing, which is a major functional regression.	High
	Treat failed branch switch as error Treat a failed branch switch during a rollback as a fatal error instead of a warning. This ensures the repository is in the correct state before checking out a specific commit, preventing potential deployment of incorrect code. api/internal/features/github-connector/service/clone_repository.go [113-119] // Checkout the specific branch first, then the commit if branch != "" { err = s.gitClient.SwitchBranch(clonePath, branch) if err != nil { - s.logger.Log(logger.Warning, fmt.Sprintf("Failed to switch to branch %s, continuing with checkout: %s", branch, err.Error()), c.UserID) + s.logger.Log(logger.Error, fmt.Sprintf("Failed to switch to branch %s for rollback: %s", branch, err.Error()), c.UserID) + return "", err } } Apply / Chat Suggestion importance[1-10]: 8 __ Why: This suggestion correctly identifies a potential bug where ignoring a failed branch switch during rollback could lead to checking out a commit on the wrong branch, causing an incorrect deployment. Treating this as a fatal error is crucial for the reliability of the rollback feature.	Medium
	Handle repository removal errors properly Improve error handling for repository removal by checking for "no such file or directory" errors specifically, and returning any other unexpected errors to prevent deployment failures. api/internal/features/github-connector/service/clone_repository.go [92-96] // Always remove existing repository to ensure fresh clone if err := s.gitClient.RemoveRepository(clonePath); err != nil { - s.logger.Log(logger.Warning, fmt.Sprintf("Failed to remove existing repository (may not exist): %s", err.Error()), c.UserID) - // Continue anyway as the directory might not exist + // It's okay if the repository doesn't exist. + // We can check for "no such file or directory" in the error message. + // This is not perfectly robust but a common practice for remote command execution. + if !strings.Contains(err.Error(), "no such file or directory") { + s.logger.Log(logger.Error, fmt.Sprintf("Failed to remove existing repository: %s", err.Error()), c.UserID) + return "", err + } + s.logger.Log(logger.Warning, fmt.Sprintf("Could not remove repository (may not exist): %s", err.Error()), c.UserID) } Apply / Chat Suggestion importance[1-10]: 7 __ Why: The suggestion correctly points out that ignoring all errors from `RemoveRepository` could lead to subsequent clone failures. Differentiating between a non-existent directory and other errors (like permissions) would make the process more robust and prevent silent failures.	Medium
High-level	Use unique paths for clones The current shared cloning directory can cause race conditions between concurrent deployments. To fix this, each deployment should use a unique directory by including the deployment ID in the path. Examples: api/internal/features/github-connector/service/authenticated_url.go [51-53] func (s GithubConnectorService) GetClonePath(userID, environment, applicationID string) (string, error) { repoBaseURL := config.AppConfig.Deployment.MountPath clonePath := filepath.Join(repoBaseURL, userID, environment, applicationID) api/internal/features/github-connector/service/clone_repository.go [83] clonePath, err := s.GetClonePath(c.UserID, c.Environment, c.ApplicationID) Solution Walkthrough: Before: // api/internal/features/github-connector/service/authenticated_url.go func (s GithubConnectorService) GetClonePath(userID, environment, applicationID string) (string, error) { repoBaseURL := config.AppConfig.Deployment.MountPath // Path is not unique per deployment clonePath := filepath.Join(repoBaseURL, userID, environment, applicationID) // ... return clonePath, nil } // api/internal/features/github-connector/service/clone_repository.go func (s GithubConnectorService) CloneRepository(c CloneRepositoryConfig, ...) (string, error) { clonePath, err := s.GetClonePath(c.UserID, c.Environment, c.ApplicationID) // ... // This can delete a directory used by another concurrent deployment s.gitClient.RemoveRepository(clonePath); s.gitClient.Clone(...) return clonePath, nil } After: // api/internal/features/github-connector/service/authenticated_url.go func (s GithubConnectorService) GetClonePath(userID, environment, applicationID, deploymentID string) (string, error) { repoBaseURL := config.AppConfig.Deployment.MountPath // Path is now unique per deployment clonePath := filepath.Join(repoBaseURL, userID, environment, applicationID, deploymentID) // ... return clonePath, nil } // api/internal/features/github-connector/service/clone_repository.go func (s *GithubConnectorService) CloneRepository(c CloneRepositoryConfig, ...) (string, error) { // Pass the unique deployment ID to generate a unique path clonePath, err := s.GetClonePath(c.UserID, c.Environment, c.ApplicationID, c.DeploymentID) // ... // This deletes a unique directory, avoiding race conditions s.gitClient.RemoveRepository(clonePath); s.gitClient.Clone(...) return clonePath, nil } Suggestion importance[1-10]: 9 __ Why: The suggestion correctly identifies a critical race condition where concurrent deployments for the same application would interfere with each other by using a shared `clonePath`, leading to unpredictable build failures.	High
Security	Quote git command arguments Prevent potential shell injection vulnerabilities by quoting the `branch`, `repoURL`, and `destinationPath` arguments in the `git clone` command string. api/internal/features/github-connector/service/git_client.go [70-73] // Use shallow clone (--depth=1) and clone only the specific branch (--branch) -cmd := fmt.Sprintf("git clone --depth=1 --branch=%s %s %s", branch, repoURL, destinationPath) +cmd := fmt.Sprintf("git clone --depth=1 --branch=\"%s\" \"%s\" \"%s\"", branch, repoURL, destinationPath) output, err := client.Run(cmd) Apply / Chat Suggestion importance[1-10]: 9 __ Why: This suggestion addresses a significant security vulnerability by recommending quoting arguments in a shell command. Unquoted user-controllable inputs like `branch`, `repoURL`, and `destinationPath` could lead to command injection, making this a critical fix.	High
Update

…de_persistance

feat: add support for faster resource pulls without much conflicts

a9a10ee

Merge branch 'feat/docker_compose_deployment' into feat/disable_deplo…

87b518d

…yment_code_persistance

raghavyuva added the nixopus-api label Dec 25, 2025

qodo-code-review bot added the Review effort 3/5 label Dec 25, 2025

raghavyuva changed the base branch from feat/docker_compose_deployment to master December 25, 2025 07:01

raghavyuva changed the base branch from master to feat/docker_compose_deployment December 25, 2025 07:01

raghavyuva changed the base branch from feat/docker_compose_deployment to feat/view_compose_pack December 25, 2025 07:03

Merge branch 'feat/view_compose_pack' into feat/disable_deployment_co…

ce94ea2

…de_persistance

raghavyuva changed the title ~~feat: add support for faster resource pulls without much conflicts~~ Dec 25, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

feat: add support for faster resource pulls without conflicts#799

feat: add support for faster resource pulls without conflicts#799
raghavyuva wants to merge 3 commits intofeat/view_compose_packfrom
feat/disable_deployment_code_persistance

raghavyuva commented Dec 25, 2025 •

edited by qodo-code-review bot

Loading

coderabbitai bot commented Dec 25, 2025

Review skipped

qodo-code-review bot commented Dec 25, 2025 •

edited

Loading

qodo-code-review bot commented Dec 25, 2025 •

edited

Loading

Examples:

Solution Walkthrough:

Before:

After:

Labels

1 participant

Uh oh!

Conversation

raghavyuva commented Dec 25, 2025 • edited by qodo-code-review bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

User description

Issue

Description

Scope of Change

Screenshot / Video / GIF (if applicable)

Related PRs (if any)

Additional Notes for Reviewers (optional)

Developer Checklist

Reviewer Checklist

PR Type

Description

Diagram Walkthrough

File Walkthrough

coderabbitai bot commented Dec 25, 2025

Review skipped

qodo-code-review bot commented Dec 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Compliance Guide 🔍

qodo-code-review bot commented Dec 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Code Suggestions ✨

Examples:

Solution Walkthrough:

Before:

After:

Labels

1 participant

raghavyuva commented Dec 25, 2025 •

edited by qodo-code-review bot

Loading

qodo-code-review bot commented Dec 25, 2025 •

edited

Loading

qodo-code-review bot commented Dec 25, 2025 •

edited

Loading