-
Notifications
You must be signed in to change notification settings - Fork 96
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
feat(docker): added docker-compose script #170
feat(docker): added docker-compose script #170
Conversation
Hey @pret3nti0u5 two things here:
|
@pret3nti0u5 , instead of modifying the original Similar thing can be done for the |
@Shruti3004 yea, I'll do that. |
@GMishx The new dockerfile isn't actually anything different from the original Dockerfile, just sets up some bind mounts to allow for development on Docker or Local Machine. But I do agree with you I will push a new commit with two Dockerfiles and docker-compose scripts. I'll keep the prod version of docker-compose to use just the prod version of Dockerfile, so that running it can be consolidated into one simple command. I'll keep the current changes for dev version of Dockerfile and docker-compose. Hope it sounds all good? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some suggestions and questions.
Renamed the Dockerfiles and docker-compose files to what was suggested. Also added Currently the project uses the entire URL described in .env (REACT_APP_SERVER_URL) which causes said CORS issues. To get rid of them we will have to proxy requests sent from the Frontend. You can read more about it here. This would require a slight rework of the current code, which I am ready to work for, but would suggest to open a new PR for resolving this issue. The benefits of this rework would be that fossology and FOSSologyUI would never need to be merged for a proper production deployment. For development purposes, the user would either have to stick only to the Docker based development or forego the usage of the Backend for development. Let me know what the proper approach to this problem would be according to you. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes looks good. Needs test.
This will be a better solution. And the
Yes, it should be a new PR.
Having a single If you are willing to work on the same, I would recommend you create an issue first for the same. |
@pret3nti0u5 , please add docker files to |
@pret3nti0u5 looks like you've merged some branch to your existing branch bringing other commits to this PR. We at FOSSology prefer rebasing over merge. It helps keep git history clean. Can you please rebase your branch with main branch? |
5be4a58
to
f36abe0
Compare
@GMishx I guess this should work? Sorry, I am a bit new when it comes to using git with multiple members pushing and pulling changes. |
Hi @pret3nti0u5, can you please squash your commits and resolve the conflicts? |
f36abe0
to
41ea40b
Compare
Hey @sjha2048, I guess this is fine? Let me know if any changes are needed! |
Hello @pret3nti0u5 , Looks like files And do you have any idea why following is happening?
|
Hey @GMishx, The docker scripts seem to be working fine on my machine, maybe it's a gitpod specific issue? Generally if you mention the dockerfile in the context directive without specifically separating it out, it still works but I'll separate it out into context and dockerfile directives in the docker-compose files, maybe it'll resolve the issue. Ill push a commit soon, let me know if you want me to squash it too. |
Should work now, works on my machine. Please test it and let me know if I should squash the commit. @GMishx @sjha2048 @Shruti3004 |
Everything seems to be working fine now. Just one suggestion to add Otherwise, things are working as expected. And please change the commit message of last commit ( |
Yeah, I didn't want to edit the original Dockerfile so I kept it as build args as specified in the original Dockerfile instead of environment variables. I'll make that change now. |
It's better to have build args. Just suggesting to add environment variables too. |
The original Dockerfile uses the build args as env variables, wouldn't defining environment variables in the compose files conflict with that? |
It won't cause issue as the So, if someone wants to update the value after they have created the image, just updating the environment in compose.yml file will work and they'll not have to re-create the images. |
Got it, let me push the changes. |
c2eb151
to
7184b3d
Compare
@pret3nti0u5 Can you please update the commit messages so I can merge this PR? |
@GMishx Could you please tell me which commit message I should update? Or do you want me to squash all those commits into one? |
7184b3d
to
baa8619
Compare
@GMishx I guess this works? Let me know if you want me to change something. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested, working as expected.
Looks good now. Thank you |
Thanks for being patient with me! Glad I could add something for the organization. |
Description
Adds a docker-compose script.
Changes
Added a docker-compose script. Changed the original Dockerfile to work better with the docker-compose script. Edited README.md to better reflect the changes and how to use the new docker-compose script.
How to test
docker-compose up -d
Please consider using the closing keyword if the pull request is proposed to
fix an issue already created in the repository
(https://help.github.com/articles/closing-issues-using-keywords/)