Skip to content

Update deploy_docker.sh #716

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

Closed
wants to merge 4 commits into from
Closed

Update deploy_docker.sh #716

wants to merge 4 commits into from

Conversation

bugy
Copy link
Owner

@bugy bugy commented Nov 7, 2023

No description provided.

@bugy bugy mentioned this pull request Nov 7, 2023
@bugy
Copy link
Owner Author

bugy commented Nov 7, 2023

@vnghia
Copy link
Collaborator

vnghia commented Nov 7, 2023

The only thing we need to check is the docker build process, which is only triggered if we create a release :/

@vnghia
Copy link
Collaborator

vnghia commented Nov 7, 2023

Ah no, so the problem is deeper than I thought. Since the Docker image is just copying the process, which is specific for amd64. I will investigate and return back to you later

@bugy
Copy link
Owner Author

bugy commented Nov 7, 2023

Sorry, which process are you talking about?

@vnghia
Copy link
Collaborator

vnghia commented Nov 7, 2023

I guess the right thing to do is having a standalone Dockerfile as in #206. We can use the multi-stage build technique to keep the image size as small as possible (probably approximately equal to the current one) so your concern in this #206 (comment) is solved

@vnghia
Copy link
Collaborator

vnghia commented Nov 7, 2023

Sorry, which process are you talking about?

Sorry, I mean the output of the CI process, the build/script-server.zip

@bugy
Copy link
Owner Author

bugy commented Nov 7, 2023

But the zip file should contain only python files, which are supposed to be platform independent

@vnghia
Copy link
Collaborator

vnghia commented Nov 7, 2023

I missed that part. Could you trigger the docker build ?

@bugy
Copy link
Owner Author

bugy commented Nov 7, 2023

@bugy
Copy link
Owner Author

bugy commented Nov 7, 2023

I'm trying to make docker image work for PRs

@bugy
Copy link
Owner Author

bugy commented Nov 7, 2023

@vnghia there is an error:

unknown flag: --platform

@bugy
Copy link
Owner Author

bugy commented Nov 7, 2023

@vnghia
Copy link
Collaborator

vnghia commented Nov 7, 2023

Is there a way I can test it on my PR ?

@bugy
Copy link
Owner Author

bugy commented Nov 7, 2023

can you try pushing any changes from your PR? I made a change to travis, probably it should build your PR now

@vnghia
Copy link
Collaborator

vnghia commented Nov 7, 2023

@vnghia
Copy link
Collaborator

vnghia commented Nov 7, 2023

Can you show me the log :/

@bugy
Copy link
Owner Author

bugy commented Nov 7, 2023

https://app.travis-ci.com/github/bugy/script-server/builds/267081647

don't you have access to this section?
image

@bugy
Copy link
Owner Author

bugy commented Nov 7, 2023

btw, I gave you access to this repo, you can try doing changes in my branch

@vnghia
Copy link
Collaborator

vnghia commented Nov 7, 2023

I see the log now, Travis blocked me while I am on VPN :/

@vnghia
Copy link
Collaborator

vnghia commented Nov 7, 2023

hmm it seems that the CI won't run if there are my trace in the PR, maybe because I am a new contributor and it won't run unless you have to approve the PR once ?

@vnghia vnghia self-requested a review November 7, 2023 19:36
@bugy
Copy link
Owner Author

bugy commented Nov 7, 2023

@vnghia I triggered the build manually and it worked! Could you try running arm docker container, please?
https://hub.docker.com/repository/docker/bugy/script-server/tags?page=1&ordering=last_updated

@vnghia
Copy link
Collaborator

vnghia commented Nov 7, 2023

It's working! Actually, you can test it on your own computer with docker run --rm -it --platform linux/arm64 -p 5000:5000 bugy/script-server:test-amd-build assuming that you have qemu installed.

@bugy
Copy link
Owner Author

bugy commented Nov 8, 2023

Cool, thanks! Could you apply the same changes to your PR please? And then we merge it

@vnghia
Copy link
Collaborator

vnghia commented Nov 8, 2023

Done !

@vnghia vnghia closed this Nov 8, 2023
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