Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: pull coolify image only when the app needs to be updated #3312

Merged
merged 3 commits into from
Sep 5, 2024

Conversation

Vahor
Copy link
Contributor

@Vahor Vahor commented Sep 4, 2024

Always use next branch as destination branch for PRs, not main

Still looking to improve #3226

While doing so, I saw that the PullCoolifyImageJob was called every hour 👀 and even if is_auto_update_enabled was disabled, it was pulling the latest docker image (doing ssh stuff etc) so not really geat.
I also saw that at the end this job might not be needed as we already have a job to Update the instance, and pulling the latest version at that moment seems a better idea ?

So what that pr does:

  • move the docker pull to the Update class
  • remove the pull image from PullCoolifyImageJob thus making it a "FetchLatestCoolifyVersionJob"
    • But as we can see, we already have a CheckForUpdatesJob that fetch the version (in the same cron interval), and could save the versions.json file. That's what I did, the PullCoolifyImageJob is entirely removed, and CheckForUpdatesJob now saves the json file.

Note: not really sure how to test this, in local I removed the isDev check and made sure that there was no error in the scheduler. I also tried to force an update by removing the $this->latestVersion === $this->currentVersion check, no issue so far. (but as it's local I might have missed something)

And question for the reviewer as I do not know the codebase well enough: Do we need to have a PullHelperImageJob and run it every hour (update_check_frequency). Or can we pull that image when upgrading the app ? (or in other words, does the helper image change outside of releases)

Copy link

sentry-io bot commented Sep 4, 2024

🔍 Existing Issues For Review

Your pull request is modifying functions with the following pre-existing issues:

📄 File: app/Console/Kernel.php

Function Unhandled Issue
App\Console\Kernel::schedule ErrorException: Attempt to read property "settings" on null /app/Console/Kernel.php in Illuminate\Foundation\Bootstrap\HandleExceptions::...
Event Count: 146k
App\Console\Kernel::schedule Error: Class "App\Jobs\Log" not found /app/Jobs/P...
Event Count: 3
App\Console\Kernel::schedule ParseError: syntax error, unexpected variable "$e" /app/Jobs/PullHelperImageJob.php in Composer\Autoload{closur...
Event Count: 1

Did you find this useful? React with a 👍 or 👎

@peaklabs-dev
Copy link
Member

@Vahor Why not just change the cron interval in the settings so change the update check frequency so that it does not happen every hour (I added it in v320, so it is new)?

@Vahor
Copy link
Contributor Author

Vahor commented Sep 4, 2024

@peaklabs-dev the 1h cron is not the issue, the issue is pulling a docker image when it's not needed, we should only pull the latest image when we want to actually upgrade the app

@Vahor Vahor marked this pull request as ready for review September 4, 2024 19:13
@Vahor Vahor force-pushed the pull-coolify-only-on-updates branch from 6139f7a to 0dad77a Compare September 4, 2024 19:25
@peaklabs-dev
Copy link
Member

Ah now, I see what you mean. Man, today is not my best day (the second time my brain just gave up). I should probably go to sleep.

@Vahor
Copy link
Contributor Author

Vahor commented Sep 4, 2024

I put a question in the description but maybe you can answer it (maybe tomorrow 😁)

Do we need to have a PullHelperImageJob and run it every hour (update_check_frequency). Or can we pull that image when upgrading the app ? (or in other words, does the helper image change outside of releases)

@andrasbacsai
Copy link
Member

andrasbacsai commented Sep 5, 2024

I have added a version controlled helper image, so the pull helper image job should check the latest version via http request (which costs way less cpu than docker pull) and if an update is available, then pull the new version.

I also test this.

@andrasbacsai andrasbacsai merged commit b241f17 into coollabsio:next Sep 5, 2024
1 check passed
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 6, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants