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

Replace the app files atomically and docker stop first before starting up for succeeding commits #70

Open
wants to merge 31 commits into
base: master
Choose a base branch
from

Conversation

asakapab0i
Copy link

@asakapab0i asakapab0i commented Jan 29, 2024

In django application, everytime a commit is added and the action triggers the script it loses the link to the original files and I think its because in the script it deletes and replace the app files.

So I added two things to ensure that the refreshed files are properly loaded.

  • Replace the app files atomically
  • Use docker restart instead of up

Let me know your thoughts.

@crohr
Copy link
Member

crohr commented Jan 29, 2024

Hi @asakapab0i, and thanks for the contribution. Could you just explain what problem you encountered in your case, due to the removal and replacement of the /app folder?

Regarding the compose restart, I'm afraid it would not work in all cases, especially if a previous commit made the stack unstable. In this case a restart is not enough to get the services back up.

@asakapab0i
Copy link
Author

Hi @asakapab0i, and thanks for the contribution. Could you just explain what problem you encountered in your case, due to the removal and replacement of the /app folder?

Regarding the compose restart, I'm afraid it would not work in all cases, especially if a previous commit made the stack unstable. In this case a restart is not enough to get the services back up.

Hi @crohr , thanks for looking into this PR,

So what happens is whenever a new commit / build is triggered it creates an error where it says the template files are not found I suspect it is because during the build process it deletes the files and recreates the app.

It does work when the docker is restarted or stopping and starting the docker-compose manually.

As for the docker restart, I figured it wouldn't work since it wouldn't be able to apply the new changes since it requires a --build parameter to rebuild.

@asakapab0i asakapab0i changed the title Replace the app files atomically and docker restart instead of up for succeeding commits Replace the app files atomically and docker stop first before starting up for succeeding commits Jan 30, 2024
@@ -89,7 +89,7 @@ def public_dns

def url
scheme = (default_port == "443" ? "https" : "http")
"#{scheme}://#{public_dns}:#{default_port}"
"#{scheme}://#{public_dns}"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

default_port should be kept if different than a standard port, but yes can be removed if 443 or 80

@crohr
Copy link
Member

crohr commented Feb 2, 2024

So what happens is whenever a new commit / build is triggered it creates an error where it says the template files are not found I suspect it is because during the build process it deletes the files and recreates the app.

When you say "it" creates an error, do you mean your application, or the pullpreview action?

It does work when the docker is restarted or stopping and starting the docker-compose manually.

Maybe we should instead docker compose stop before updating code, but I never encountered this issue. Would be nice to get some more detail so that I can reproduce.

@asakapab0i
Copy link
Author

asakapab0i commented Feb 5, 2024

When you say "it" creates an error, do you mean your application, or the pullpreview action?

The application fails to recognize the change in the application files. It says it cannot find the expected template files.

Maybe we should instead docker compose stop before updating code, but I never encountered this issue. Would be nice to get some more detail so that I can reproduce.

I'll spend some time reproducing it and maybe it just works for all other applications except ours, if that's the case I'll just include the docker stop on this PR.

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.

2 participants