Skip to content

Conversation

@rhysyngsun
Copy link
Contributor

@rhysyngsun rhysyngsun commented Jun 26, 2024

What are the relevant tickets?

Closes https://github.com/mitodl/hq/issues/4602
Closes https://github.com/mitodl/hq/issues/4611

Description (What does it do?)

  • All nginx serving of frontend is removed (except for the e2e tests as a special case).
  • Updated the nginx setup in docker-compose to use the same nginx.conf that production uses. This implementation was lifted from MITx Online
  • Changed the configuration of env_file so that it's split up between frontend vs. backend so it's generally cleaner and not as noisy.
  • Renamed the environment variables for the API and app urls so they have more obvious names.

How can this be tested?

  • You should be able to still use your existing .env file as I left support for that in place
  • You can also split it up into env/frontend.local.env and env/backend.local.env. There's also env/shared.local.env, but that is mainly for the MITOPEN_API_BASE_URL and MITOPEN_APP_BASE_URL vars.
  • You can also remove a lot of vars from your .env file(s) that are now defined by default in the checked in .env files. A lot of time we just ended up leaving what was in .env and this should hopefully make that file a lot less noisy.
  • You should be able to do all the normal things you can do:
    • Run the app via docker compose up with the different combinations of COMPOSE_PROFILES set.
      • If you're running both profiles, you'll now access the app in your browser via port 8062 (e.g. http://open.odl.local:8062/). The API and django-admin are still on port 8063.
    • Run one-off containers via docker compose run --rm web bash, etc.
    • Run the tests on both frontend and backend.

@rhysyngsun rhysyngsun force-pushed the nl/backend-remove-frontend-serve branch from 38fd7f8 to 8545b1a Compare June 27, 2024 02:04
@rhysyngsun rhysyngsun force-pushed the nl/backend-remove-frontend-serve branch from 74ff313 to 9b9d8b6 Compare June 27, 2024 02:12
@rhysyngsun rhysyngsun added product:mit-open Issues related to the MIT Open product Needs Review An open Pull Request that is ready for review and removed product:mit-open Issues related to the MIT Open product labels Jun 27, 2024
@rhysyngsun rhysyngsun marked this pull request as ready for review June 27, 2024 14:40
Copy link
Contributor

@gumaerc gumaerc left a comment

Choose a reason for hiding this comment

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

I was able to get this working, but I had to make some tweaks to the nginx configuration:

@gumaerc gumaerc self-assigned this Jun 27, 2024
@gumaerc gumaerc added Waiting on author and removed Needs Review An open Pull Request that is ready for review labels Jun 27, 2024
@gumaerc gumaerc assigned rhysyngsun and unassigned gumaerc Jun 27, 2024
@rhysyngsun rhysyngsun force-pushed the nl/backend-remove-frontend-serve branch from 185a01b to c0cdeb0 Compare June 27, 2024 21:52
@rhysyngsun rhysyngsun added Needs Review An open Pull Request that is ready for review and removed Waiting on author labels Jun 27, 2024
@rhysyngsun
Copy link
Contributor Author

@gumaerc I addressed your feedback and also updated the readme to reflect the new config file locations.

Copy link
Contributor

@gumaerc gumaerc left a comment

Choose a reason for hiding this comment

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

This looks good to me now, however I will leave you a reminder to put up a PR in ol-infrastructure to set MITOPEN_APP_BASE_URL and MITOPEN_API_BASE_URL in our various deployment environments before releasing this.

@gumaerc gumaerc added Waiting on author and removed Needs Review An open Pull Request that is ready for review labels Jun 28, 2024
Copy link
Member

@mbertrand mbertrand left a comment

Choose a reason for hiding this comment

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

Using my old .env, I can't seem to login via django admin (tried via both ports/domains), I always get a CSRF error, unless I set both MITOPEN_API_BASE_URL and MITOPEN_APP_BASE_URL to localhost with different ports. Even setting them both to open.odl.local with different ports leads to a CSRF error.

Renamed my old .env and tried to use env/backend.local.env, env/shared.local/end, frontend.local.env but it couldn't seem to find shared.local.env at least where COMPOSE_PROFILES is, kept getting the error "no service selected"

@gumaerc
Copy link
Contributor

gumaerc commented Jun 28, 2024

@rhysyngsun I will test this again on Monday and take a closer look at the settings Matt mentioned above, but we should probably hold off merging until we know it picks up those settings properly.

@rhysyngsun
Copy link
Contributor Author

Renamed my old .env and tried to use env/backend.local.env, env/shared.local/end, frontend.local.env but it couldn't seem to find shared.local.env at least where COMPOSE_PROFILES is, kept getting the error "no service selected"

COMPOSE_PROFILES still needs to be defined in a root .env file or as an exported environment variable in your shell. This is because the behavior of it being picked up in that file is a feature of docker compose. We just happened to be also using this file for our own .env. I'll update the readme to make that clear and remove COMPOSE_PROFILES from the shared.local.example.env file.

@@ -0,0 +1,2 @@
# MITOPEN_AXIOS_WITH_CREDENTIALS=True
# WEBPACK_ANALYZE=True
Copy link
Contributor

Choose a reason for hiding this comment

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

EMBEDLY_KEY= here as well.

MITOPEN_AXIOS_WITH_CREDENTIALS isn't needed as it's in the committed file. Are the .local.env files intended only for secrets (aside from where it makes sense to group related variables)? Thinking WEBPACK_ANALYZE can go in frontend.env as well.

@gumaerc
Copy link
Contributor

gumaerc commented Jul 1, 2024

Renamed my old .env and tried to use env/backend.local.env, env/shared.local/end, frontend.local.env but it couldn't seem to find shared.local.env at least where COMPOSE_PROFILES is, kept getting the error "no service selected"

COMPOSE_PROFILES still needs to be defined in a root .env file or as an exported environment variable in your shell. This is because the behavior of it being picked up in that file is a feature of docker compose. We just happened to be also using this file for our own .env. I'll update the readme to make that clear and remove COMPOSE_PROFILES from the shared.local.example.env file.

I tested this again today and confirmed that COMPOSE_PROFILES needs to be set at the root level, but the other vars pull just fine from the more specific env files.

@rhysyngsun rhysyngsun force-pushed the nl/backend-remove-frontend-serve branch from c0cdeb0 to 4db5c53 Compare July 1, 2024 19:50
Copy link
Member

@mbertrand mbertrand left a comment

Choose a reason for hiding this comment

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

No longer getting a CSRF error

env/frontend.env Outdated
NODE_ENV=development
PORT=8062
MITOPEN_AXIOS_WITH_CREDENTIALS=true
WEBPACK_ANALYZE=True
Copy link
Member

Choose a reason for hiding this comment

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

This causes my watch container to throw an error and exit

WEBPACK_ANALYZE: Invalid bool input: "True"

Copy link
Contributor

Choose a reason for hiding this comment

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

This causes my watch container to throw an error and exit

WEBPACK_ANALYZE: Invalid bool input: "True"

I get this same error

Copy link
Contributor

Choose a reason for hiding this comment

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

@rhysyngsun I think this needs to be lowercase true because the var is evaluated by Javascript

@rhysyngsun rhysyngsun force-pushed the nl/backend-remove-frontend-serve branch from 4db5c53 to 44464ef Compare July 1, 2024 21:37
Copy link
Member

@mbertrand mbertrand left a comment

Choose a reason for hiding this comment

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

👍

@rhysyngsun rhysyngsun force-pushed the nl/backend-remove-frontend-serve branch from 1b9f193 to c1d0c9c Compare July 2, 2024 15:03
@rhysyngsun rhysyngsun merged commit 1f82492 into main Jul 2, 2024
@rhysyngsun rhysyngsun deleted the nl/backend-remove-frontend-serve branch July 2, 2024 15:14
This was referenced Jul 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants