-
Notifications
You must be signed in to change notification settings - Fork 22
feature: Added ability to run OCIM in a container. #795
Conversation
372b047
to
2e2c19c
Compare
@kewne Thank you for doing this. I made a quick pass over it and added some comments. |
2e2c19c
to
db3e865
Compare
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.
Thanks for this! I haven't tested it, but I'm happy to see it :-)
Let me know if you need an additional reviewer/tester, and I can do that.
7fd6eed
to
796a37f
Compare
@kewne I'm getting this error when trying to run migrate:
Also, looks like this needs a rebase, as master has changed. |
796a37f
to
2903d7e
Compare
@Kelketek argh, the |
@kewne Probably not. Chances are it was needed when first implementing the angular interface, but once it was no longer needed, no one bothered to remove the -e. I didn't even realize the package exists until it failed for me. So go ahead and try removing the -e. We can look at re-adding the -e if we find a need for it and it can be on the person who needs it to make it work with Docker once we have that as a standard feature for OCIM. :) |
@kewne I can review this, but probably not until Tuesday. |
2903d7e
to
86e06a0
Compare
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.
I followed the docs and managed to get myself set up on an m1 mac. Thanks.
86e06a0
to
e2f1e4f
Compare
@Kelketek @bradenmacdonald I've fixed the @MartinKaburu in case you need to develop on top of this before it gets merged in, you can use this workflow: git checkout my_work_branch
# rebase changes on top of docker
git rebase joaocabrita/ocim_in_docker
# hack, hack hack...
# when you're done:
git fetch
# rebase all commits in my_work_branch but not in joaocabrita/ocim_in_docker on top of origin/master
git rebase --onto origin/master joaocabrita/ocim_in_docker my_work_branch |
@kewne This is working for me now. I notice that the management backend is the UI on 5000-- I think 3000 is where the customer-facing UI is supposed to be-- are there special directions for how to start this up in the docker environment? |
da6bd4f
to
1d3cb12
Compare
@Kelketek The frontend should now be working as well; I added the instructions to the documentation but the workflow is a bit clunky because |
@kewne Much closer now!
I've forced a rebuild of the items and removed node_modules before restarting, but I keep getting this error when beginning from scratch. Also it would be good to make this container set rootless. https://pythonspeed.com/articles/root-capabilities-docker-security/ since I found I had to use sudo to remove all of the node_modules files (they were owned by root). There are some other potential solutions to the awkwardness of syncing node_modules-- many available if you Google around, since lots of people have this problem, but I don't think it needs to be in this PR. What you're doing is probably sufficient there. |
@Kelketek can you try
I'd like to tackle that as part of (at least) one separate PR/story making containerized OCIM production ready; |
@kewne Please create that follow-up ticket for the rootless container since I'm afraid it may get overlooked otherwise, and we do intend to containerize OCIM fully, so this work might end up the basis of something much larger. There should be an OCIM docker epic somewhere you can attach the ticket to. Ping the current owner on it to make them aware of the creation. I left one comment. Once addressed, this is 👍 from me. No need for me to fiddle with it again. |
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.
👍 if you fix the frontend build issue I encountered.
- I tested this: started it up locally
- I read through the code
- I checked for accessibility issues: n/a
- Includes documentation: yes, thx
992d1c9
to
914ff37
Compare
@Kelketek @bradenmacdonald I've created https://tasks.opencraft.com/browse/BB-4246 for the follow up work that came up during the review. Since I've addressed all of your comments, I'll merge this once it passes all checks; thanks for your patience! |
@kewne Looks like you'll need to do a commit --amend and force push to retrigger the tests. Also please check in the OCIM Mattermost channel to see if this failure is a known one that we're already planning on handling. I think this might be something we already have a ticket to fix and is unrelated to your changes, but I'm not sure. |
@Kelketek it's failing on master as well: https://app.circleci.com/pipelines/github/open-craft/opencraft/3742/workflows/75ea8813-4744-4c7c-b198-260d5bbc0f16/jobs/33686 |
914ff37
to
7c92ea8
Compare
7c92ea8
to
9460dcb
Compare
@Kelketek @bradenmacdonald Seems like master is (finally) green again, so I'll be merging this in as soon as I get the flakes to pass. |
@kewne Oh man, I thought this was already merged. Better late than never though. Thanks!!! |
Description
This adds some basic infrastructure for developing OCIM inside docker containers.
It already makes it possible to run some of the dev workflow (e.g. tests and OCIM) in Docker and could be used as a starting point for running a dockerized OCIM in production.
Kudos to @janimo for the prior work on #740
Testing
Follow the instructions in docker.md