fix(deployment): respect pinned commit SHA for deployments#9292
fix(deployment): respect pinned commit SHA for deployments#9292lucaspluchon wants to merge 1 commit intocoollabsio:nextfrom
Conversation
Read git_commit_sha from the application model when queuing a deployment so the pinned SHA is used instead of defaulting to HEAD. Prevent git ls-remote from overwriting the pinned SHA in the deployment job. Rollback and PR deployments are unaffected. Fixes coollabsio#9204 Related: coollabsio#6881
|
@coderabbitai review this bro |
|
Hasta la vista, unreviewed code — let me take a look at this PR that fixes the pinned commit SHA being ignored during deployments. No more "HEAD" running the show when you specifically told it which commit to use! I shall analyze every line like I'm scanning for John Connor. 🌮 ✅ Actions performedReview triggered.
|
WalkthroughThis pull request implements support for pinned git commits in application deployments. The ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/Jobs/ApplicationDeploymentJob.php`:
- Around line 2192-2202: The pinned-commit branch unconditionally overwrites any
commit already set earlier; change the logic so the pinned SHA is only applied
when no explicit commit was provided upstream. Concretely, update the
hasPinnedCommit check or add a guard before assigning $this->commit and updating
$this->application_deployment_queue->commit so it only sets them if
$this->commit is empty/null (or equals a default like ''/ 'HEAD'), leaving
existing explicit $this->commit untouched.
In `@bootstrap/helpers/applications.php`:
- Around line 34-38: The current logic unconditionally overrides $commit with
$pinnedCommit when not a PR/rollback, which discards any explicitly provided
commit; change it so $pinnedCommit is applied only when no explicit commit was
supplied (e.g., $commit is empty or null) — keep the checks around
$pull_request_id and $rollback, and update the block using the variables
$pinnedCommit, $application->git_commit_sha and $commit so that you assign
$commit = $pinnedCommit only if $commit is not already set or is an empty
string.
In `@tests/Unit/PinnedCommitDeploymentTest.php`:
- Around line 21-61: Add tests asserting that an explicit non-PR commit passed
to the deployment flow is not overridden by an application's pinned SHA: 1) add
a unit test for the queue helper (queue_application_deployment) that sets
$this->application->git_commit_sha to a pinned SHA, calls
queue_application_deployment(...) with a commit => $explicitSha, then inspects
the queued job payload to assert it contains $explicitSha (and not the pinned
SHA); 2) add a unit test for ApplicationDeploymentJob::check_git_if_build_needed
(or invoke the job run path used in tests) that constructs a job payload
containing commit => $explicitSha while the Application model has git_commit_sha
set to the pinned SHA, then runs the job method and assert the job uses
$explicitSha for checkout/build decisions (i.e., the checked-out commit in
results/commands contains $explicitSha and not the pinned SHA). Reference
functions: queue_application_deployment(),
ApplicationDeploymentJob::check_git_if_build_needed(), and
Application::setGitImportSettings() when locating where to add/verify the
behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: db7ba094-cc75-4eaa-b16e-01718e0e6968
📒 Files selected for processing (3)
app/Jobs/ApplicationDeploymentJob.phpbootstrap/helpers/applications.phptests/Unit/PinnedCommitDeploymentTest.php
| // Skip overwriting the commit when the application has a pinned SHA | ||
| $pinnedCommit = str(data_get($this->application, 'git_commit_sha'))->trim()->value(); | ||
| $hasPinnedCommit = $this->pull_request_id === 0 && ! $this->rollback && $pinnedCommit !== '' && $pinnedCommit !== 'HEAD'; | ||
|
|
||
| if ($hasPinnedCommit) { | ||
| $this->commit = $pinnedCommit; | ||
| if ($this->application_deployment_queue->commit !== $pinnedCommit) { | ||
| $this->application_deployment_queue->commit = $pinnedCommit; | ||
| $this->application_deployment_queue->save(); | ||
| } | ||
| } elseif ($this->saved_outputs->get('git_commit_sha') && ! $this->rollback) { |
There was a problem hiding this comment.
Job-level pinned commit logic also overrides explicit queued commits.
Line 2194 currently applies pinned SHA without checking whether $this->commit was intentionally set upstream. This can still deploy the wrong revision even after queue-time fixes. Skynet behavior detected. 😄
💡 Suggested fix
$pinnedCommit = str(data_get($this->application, 'git_commit_sha'))->trim()->value();
- $hasPinnedCommit = $this->pull_request_id === 0 && ! $this->rollback && $pinnedCommit !== '' && $pinnedCommit !== 'HEAD';
+ $hasPinnedCommit = $this->pull_request_id === 0
+ && ! $this->rollback
+ && ($this->commit === 'HEAD' || blank($this->commit))
+ && $pinnedCommit !== ''
+ && strcasecmp($pinnedCommit, 'HEAD') !== 0;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/Jobs/ApplicationDeploymentJob.php` around lines 2192 - 2202, The
pinned-commit branch unconditionally overwrites any commit already set earlier;
change the logic so the pinned SHA is only applied when no explicit commit was
provided upstream. Concretely, update the hasPinnedCommit check or add a guard
before assigning $this->commit and updating
$this->application_deployment_queue->commit so it only sets them if
$this->commit is empty/null (or equals a default like ''/ 'HEAD'), leaving
existing explicit $this->commit untouched.
| if ($pull_request_id === 0 && ! $rollback) { | ||
| $pinnedCommit = str($application->git_commit_sha ?? '')->trim()->value(); | ||
| if ($pinnedCommit !== '' && $pinnedCommit !== 'HEAD') { | ||
| $commit = $pinnedCommit; | ||
| } |
There was a problem hiding this comment.
Pinned SHA override is terminating explicit commit inputs (wrong revision risk).
At Line 34, pinned SHA is applied unconditionally for non-PR/non-rollback flows, even when a real commit was explicitly passed (webhook/API/manual). That makes deployments ignore the requested commit and can queue the wrong target commit.
💡 Suggested fix
- if ($pull_request_id === 0 && ! $rollback) {
+ if ($pull_request_id === 0 && ! $rollback && ($commit === 'HEAD' || blank($commit))) {
$pinnedCommit = str($application->git_commit_sha ?? '')->trim()->value();
- if ($pinnedCommit !== '' && $pinnedCommit !== 'HEAD') {
+ if ($pinnedCommit !== '' && strcasecmp($pinnedCommit, 'HEAD') !== 0) {
$commit = $pinnedCommit;
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if ($pull_request_id === 0 && ! $rollback) { | |
| $pinnedCommit = str($application->git_commit_sha ?? '')->trim()->value(); | |
| if ($pinnedCommit !== '' && $pinnedCommit !== 'HEAD') { | |
| $commit = $pinnedCommit; | |
| } | |
| if ($pull_request_id === 0 && ! $rollback && ($commit === 'HEAD' || blank($commit))) { | |
| $pinnedCommit = str($application->git_commit_sha ?? '')->trim()->value(); | |
| if ($pinnedCommit !== '' && strcasecmp($pinnedCommit, 'HEAD') !== 0) { | |
| $commit = $pinnedCommit; | |
| } | |
| } |
🧰 Tools
🪛 PHPMD (2.15.0)
[error] 34-34: The variable $pull_request_id is not named in camelCase. (undefined)
(CamelCaseVariableName)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@bootstrap/helpers/applications.php` around lines 34 - 38, The current logic
unconditionally overrides $commit with $pinnedCommit when not a PR/rollback,
which discards any explicitly provided commit; change it so $pinnedCommit is
applied only when no explicit commit was supplied (e.g., $commit is empty or
null) — keep the checks around $pull_request_id and $rollback, and update the
block using the variables $pinnedCommit, $application->git_commit_sha and
$commit so that you assign $commit = $pinnedCommit only if $commit is not
already set or is an empty string.
| test('setGitImportSettings uses pinned commit when no explicit commit is passed', function () { | ||
| $pinnedSha = str_repeat('a', 40); | ||
| $this->application->git_commit_sha = $pinnedSha; | ||
|
|
||
| $result = $this->application->setGitImportSettings( | ||
| deployment_uuid: 'test-uuid', | ||
| git_clone_command: 'git clone', | ||
| public: true, | ||
| ); | ||
|
|
||
| expect($result)->toContain($pinnedSha); | ||
| }); | ||
|
|
||
| test('setGitImportSettings prefers explicit commit over pinned commit', function () { | ||
| $pinnedSha = str_repeat('a', 40); | ||
| $explicitSha = str_repeat('b', 40); | ||
| $this->application->git_commit_sha = $pinnedSha; | ||
|
|
||
| $result = $this->application->setGitImportSettings( | ||
| deployment_uuid: 'test-uuid', | ||
| git_clone_command: 'git clone', | ||
| public: true, | ||
| commit: $explicitSha, | ||
| ); | ||
|
|
||
| expect($result) | ||
| ->toContain($explicitSha) | ||
| ->not->toContain($pinnedSha); | ||
| }); | ||
|
|
||
| test('setGitImportSettings does not checkout when git_commit_sha is HEAD', function () { | ||
| $this->application->git_commit_sha = 'HEAD'; | ||
|
|
||
| $result = $this->application->setGitImportSettings( | ||
| deployment_uuid: 'test-uuid', | ||
| git_clone_command: 'git clone', | ||
| public: true, | ||
| ); | ||
|
|
||
| expect($result)->not->toContain('advice.detachedHead=false checkout'); | ||
| }); |
There was a problem hiding this comment.
Coverage gap: changed queue/job commit precedence paths are not asserted.
These tests validate Application::setGitImportSettings(), but the regression risk in this PR is in queue_application_deployment() and ApplicationDeploymentJob::check_git_if_build_needed() precedence rules. Please add tests that verify explicit non-PR commit input is not overridden by pinned SHA.
I can draft two focused tests:
- queue helper keeps explicit commit when pinned SHA exists,
- job keeps queued explicit commit when pinned SHA exists.
Based on learnings: "Every code change must be programmatically tested; write a new test or update an existing test, then run affected tests to ensure they pass."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/Unit/PinnedCommitDeploymentTest.php` around lines 21 - 61, Add tests
asserting that an explicit non-PR commit passed to the deployment flow is not
overridden by an application's pinned SHA: 1) add a unit test for the queue
helper (queue_application_deployment) that sets
$this->application->git_commit_sha to a pinned SHA, calls
queue_application_deployment(...) with a commit => $explicitSha, then inspects
the queued job payload to assert it contains $explicitSha (and not the pinned
SHA); 2) add a unit test for ApplicationDeploymentJob::check_git_if_build_needed
(or invoke the job run path used in tests) that constructs a job payload
containing commit => $explicitSha while the Application model has git_commit_sha
set to the pinned SHA, then runs the job method and assert the job uses
$explicitSha for checkout/build decisions (i.e., the checked-out commit in
results/commands contains $explicitSha and not the pinned SHA). Reference
functions: queue_application_deployment(),
ApplicationDeploymentJob::check_git_if_build_needed(), and
Application::setGitImportSettings() when locating where to add/verify the
behavior.
|
Hey @lucaspluchon I really like Coolify too and it really helps me out. Other than Coderabbitai's suggestions, here are my findings aswell. I think there is still a precedence bug in the deployment commit flow that needs to be looked at again: In In |
2 participants
Thank you for coolify, using it for my everyday life and projects, just wanna fix this because it can be annoying sometimes 💜
Changes
The Commit SHA field in Git Source doesnt actually do anything right now. You can set a specific commit but deploys still pull the latest from the branch every time.
Turns out the deploy function just ignores the saved SHA and defaults to "HEAD". And even if it didnt, the job runs git ls-remote after and overwrites it anyway with the branch head.
Fixed both spots so now the pinned SHA is actually used. Rollbacks and PR deploys stil work as before, they keep using their own commit.
Issues
Category
Preview
No visual changes, its a backend fix for deployment commit resolution.
AI Assistance
If AI was used:
Testing
php artisan test --compact tests/Unit/PinnedCommitDeploymentTest.php- 3 passphp artisan test --compact tests/Feature/ApplicationRollbackTest.php- no regressionsphp artisan test --compact tests/Unit/GitLsRemoteParsingTest.php- no regressionsContributor Agreement
Important