-
Notifications
You must be signed in to change notification settings - Fork 10
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
I465 Adding django backend #492
Conversation
mysqlclient==2.2.6 | ||
|
||
#Django | ||
Django==5.1.4 |
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.
Django 5.1 isn't LTS, 5.2 LTS doesn't come out until April. You may want to hold back to Django 4.2 (currently 4.2.18) until later this year unless this isn't going to be released until after April 2025.
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 feel Releasing in Apr is very optimistic. I feel this is going to live in summer. Thats why I chose Django 5 version. I can change to 4 version if you like. LMK
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 think it's fine based on the release timeline here to leave it 5.1, just that we should update to Django 5.2 before we do release. It should be out in early April. I think this is good progress and we might have this by late April or May.
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 will keep 5.1 and will upgrade to 5.2 before release. I will create an issue for that.
requirements.txt
Outdated
django-mysql==4.15.0 | ||
django-webpack-loader==3.1.1 | ||
|
||
debugpy==1.8.8 |
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.
This should be updated to 1.8.11, also might want to check the versions of the other dependencies here?
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.
ok
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.
Done
using fallbacks when available and throwing an error if required values are not present. | ||
The application will exit if any of the required configuration values are not set properly. | ||
|
||
#### Canvas |
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 think much of this section is still relevant. Might want to break it out the parts that won't change into separate .MD files to make it easier to maintain separately like we did for MyLA in docs
.
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.
Yes, but I want to add stuff, based on the implementation. So I did not implement the LTI, Canvas Oauth. So I. removed it for making the PR reviewer just focus on what is there. I will add them back when I am adding features on at a time
docker-compose.yml
Outdated
environment: | ||
- PORT=4000 | ||
- NODE_ENV=development | ||
env_file: .env | ||
- DEBUG=True |
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 found out with ROHQ that you can use this pattern to specify defaults in the docker-compose file
- DEBUG=${DEBUG:-True}
I'd switch to this here for everything in the environment section.
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.
Done
docker-compose.yml
Outdated
context: ./ccm_web/ | ||
dockerfile: ./Dockerfile | ||
context: . | ||
dockerfile: Dockerfile | ||
args: |
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 don't think the args are needed here anymore.
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, Removed
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.
Approving but left a few comments which can be addressed now or in future issues if you just want to move this along. Thanks!
This PR Finishes #466, |
e754993
to
f76e363
Compare
Looks good to me, locally ran it with 404 errors on the API, which I assume is expected. I can help with APIs once we move on to that step, just let me know. |
Yes, API endpoints are not wired up so those errors are expected. We are progressively building it. This PR is to separate the FE from the Node setup and build using on Webpack and make the Js Dump file When CCM launch it makes API call, so I have to set up some static JSON response and launching the home page so that It acceptable for this PR. |
Thanks @jonespm and @jaydonkrooss for the Quick Review. |
Fixes #465
This PR is
package.json
.docker compose build
anddocker compose up
.env
file in not place in the project root directory instead in the${HOME}/secrets/ccm/.env
. So create folder/secrets/ccm
The CCM homepage will launch, but for now I added dummy data for the API call when the home page launches. This setup is going to be temporary until be have API integration happens. All the changes can be found in
api.ts
This PR does not have
ExperimentalWarning: --experimental-loader may be removed in the future; instead use register()
That is because CCM migrated to ESM and not commonJS. So in future version of Node
commonJS
support might be removed.node --loader ts-node/esm node_modules/.bin/webpack --config webpack/webpack.dev.ts --watch
4. More On this can be found here. This is something know during ##330 Upgrade CCM To ESM #433
server
. I will remove it in future.