Skip to content
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

Dev-mode #22748

Merged
merged 3 commits into from
Oct 18, 2024
Merged

Dev-mode #22748

merged 3 commits into from
Oct 18, 2024

Conversation

KevinMind
Copy link
Contributor

@KevinMind KevinMind commented Oct 9, 2024

Relates to: mozilla/addons#15066
Relates to: mozilla/addons#15046 (removes one of the mounts that could cause race conditions)
Fixes: mozilla/addons#15058

Description

Adds a DEV_MODE boolean setting based on the value of DOCKER_TARGET being not equal to production. This value is then used in the places that previously relied on DEBUG bu that should instead rely specifically on if we are using a production image or not.

Context

DEBUG has largley been used to determine if we are in "dev" or "prod" mode. But we have a better value set during the docker build DOCKER_TARGET that actually controls which set of dependencies we have and how we setup our docker compose configuration and this value is much more suited to the task of making such a determination.

This split also means you can now run prod mode in debug mode, great for debugging a production like environment.s

To put into context

Testing

There are 4 combinations of make up to run DEBUG=True/False and DOCKER_TARGET=development/production

Dev Mode

First run dev mode without debugging

make up DEBUG= DOCKER_TARGET=development

This is running the development image, with debug false. You should expect to have development dependencies installed, but running the app will not load django debug toolbar.

Now rerun the same command but change DEBUG=True (it actually can be any value, just not literally empty)

make up DEBUG=True DOCKER_TARGET=development

Prod Mode

Very much like above, except we are targeting the production image

make up DEBUG= DOCKER_TARGET=production

Now expect dev dependencies to NOT be installed. You can test by running pip show <dependency for a dep in the dev.txt file.

Rerun with debugging

make up DEBUG=True DOCKER_TARGET=production

Expect now to have debugging enabled. You are still running production mode, but debugging is now enabled.

Expectations

All four should work as expected.

  • containers running and healthy
  • endpoints serving correctly app rendering
  • make check works.
  • if debug is true, expect debug toolbar
  • if dev mode expect development dependencies.

Checklist

  • Add #ISSUENUM at the top of your PR to an existing open issue in the mozilla/addons repository.
  • Successfully verified the change locally.
  • The change is covered by automated tests, or otherwise indicated why doing so is unnecessary/impossible.
  • Add before and after screenshots (Only for changes that impact the UI).
  • Add or update relevant docs reflecting the changes made.

@KevinMind KevinMind force-pushed the dev-mode branch 2 times, most recently from 94e00bb to ffa9bf4 Compare October 9, 2024 14:45
@KevinMind KevinMind requested review from a team and eviljeff and removed request for a team October 9, 2024 15:34
@KevinMind KevinMind marked this pull request as ready for review October 9, 2024 15:34
Copy link
Member

@eviljeff eviljeff left a comment

Choose a reason for hiding this comment

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

I've been trying to think through the scenarios but I'm a bit muddled around what determines a scenario when we're in DEV_MODE v.s DEBUG (mode). Things like fake fxa auth are definitely DEV_MODE; enabling debug_toolbar or retaining temporary files for debugging purposes ... 🤷 ?

src/olympia/lib/jingo_minify_helpers.py Outdated Show resolved Hide resolved
docker-bake.hcl Show resolved Hide resolved
Makefile-docker Outdated Show resolved Hide resolved
settings.py Show resolved Hide resolved
src/olympia/lib/settings_base.py Outdated Show resolved Hide resolved
src/olympia/users/tasks.py Outdated Show resolved Hide resolved
docs/topics/development/static-files.md Outdated Show resolved Hide resolved
@KevinMind
Copy link
Contributor Author

I've been trying to think through the scenarios but I'm a bit muddled around what determines a scenario when we're in DEV_MODE v.s DEBUG (mode). Things like fake fxa auth are definitely DEV_MODE; enabling debug_toolbar or retaining temporary files for debugging purposes ... 🤷 ?

I think it's easier to know what should be DEV_MODE. As you've said fxa should be disabled in production. All production images have DOCKER_TARGET "production". boom.

debug_toolbar is a dev dependency, only possible to use it if the target is not production, hence, DEV_MODE.

Debug seems to make sense if you really think it through, as you've pointed out a few of my misses. If the feature is "useful" in a debug scenario, we can include it based on DEBUG.

DEV_MODE is about what is mandatory or what is possible, DEBUG is about what is useful.

I can run DEBUG in dev mode or prod mode, that's why it's independent. DEV_MODE is about enabling/disabling behaivours that are tightly coupled to the DOCKER_TARGET equaling production or not.

@KevinMind KevinMind force-pushed the dev-mode branch 2 times, most recently from 140cdb1 to 8435e56 Compare October 10, 2024 08:16
@KevinMind
Copy link
Contributor Author

@eviljeff I've udpated the docs to hopefully clarify the use cases for DEBUG vs DEV_MODE and fixed all commented issues.

@eviljeff
Copy link
Member

here are 4 combinations of make up to run DEBUG=True/False and DOCKER_TARGET=development/production

All four should work as expected.

* containers running and healthy

* endpoints serving correctly app rendering

* `make check` works.

* if debug is true, expect debug toolbar

to make sure I'm testing it correctly, can you state precisely the commands I should be running for the 4 combinations (and what I should expect a vanilla make up to do)

@KevinMind
Copy link
Contributor Author

KevinMind commented Oct 10, 2024

here are 4 combinations of make up to run DEBUG=True/False and DOCKER_TARGET=development/production
All four should work as expected.

* containers running and healthy

* endpoints serving correctly app rendering

* `make check` works.

* if debug is true, expect debug toolbar

to make sure I'm testing it correctly, can you state precisely the commands I should be running for the 4 combinations (and what I should expect a vanilla make up to do)

@eviljeff done. "vanilla make up doesn't really exist. It always runs with the current values in the .env If you run it with DOCKER_TARGET=banana, the next time you run "vanilla" make up, the docker target will still be banana. So since it is contextual on the current values of the .env file there is no real vanilla version, except if you remove the .env in which case docker target will be development and debug will be whatever you set debug to.

Copy link
Member

@eviljeff eviljeff left a comment

Choose a reason for hiding this comment

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

make up DEBUG= DOCKER_TARGET=production resulted in the statics not working (html served without js and css).

Other than that, the other 3 scenarios worked, at least for my brief manual tests.

@@ -1165,7 +1165,7 @@ def extract_colors_from_image(path):
def use_fake_fxa():
"""Return whether or not to use a fake FxA server for authentication.
Should always return False in production"""
return settings.DEBUG and settings.USE_FAKE_FXA_AUTH
return settings.DEV_MODE or settings.USE_FAKE_FXA_AUTH
Copy link
Member

Choose a reason for hiding this comment

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

This wasn't connected to the statics problem I found, was it? This change means you can't use real FxA auth in dev mode.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, I just needed that for local testing.. I will drop that before merging. THough it does kind of suck. Without that it mneans you HAVE to use real FxA in prod mode, which kind of also sucks. I think the real fix is to only rely on the USE_FAKE_FXA_AUTH and just make sure we hardcode production and base settings to prefer real auth. Or maybe rely on the SITE_URL or something.

Copy link
Member

Choose a reason for hiding this comment

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

Probably out of scope for this PR, but we could rely on the client_id/secret being set to a real value - the default unless you've defined credentials in your local_settings or env is .. Otherwise, redirecting to real FxA just ends in an error page.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a great idea. Yeah should be a separate ticket but that makes a lot more sense, if you pass credentials, use em.

@KevinMind KevinMind force-pushed the dev-mode branch 3 times, most recently from e2d9782 to cfe4ba6 Compare October 11, 2024 17:18
@KevinMind
Copy link
Contributor Author

make up DEBUG= DOCKER_TARGET=production resulted in the statics not working (html served without js and css).

Other than that, the other 3 scenarios worked, at least for my brief manual tests.

@eviljeff The issue is ultimately with the volume(s) we are using to load data in the nginx container. I resolved this by simplifying further the setup, removing the data_site_static volume. We had multiple volumes trying to load state to the same directory, and this exposed edge cases where the prod assets were not where they should have been.

Now nginx will try to load files from the ./static directory loaded directly in nginx (no sharing the volume) or by proxying the request to olympia to return the file. Concurrently, the logic toggling whether we serve static assets from django is no longer based on either DEBUG or DEV_MODE but instead on a dedicated setting that is True outside of production. I've tested all four scenarios after this change and everything works. LMK if it fixes the issue for you.

@eviljeff
Copy link
Member

make up DEBUG= DOCKER_TARGET=production resulted in the statics not working (html served without js and css).
Other than that, the other 3 scenarios worked, at least for my brief manual tests.

@eviljeff The issue is ultimately with the volume(s) we are using to load data in the nginx container. I resolved this by simplifying further the setup, removing the data_site_static volume. We had multiple volumes trying to load state to the same directory, and this exposed edge cases where the prod assets were not where they should have been.

Now nginx will try to load files from the ./static directory loaded directly in nginx (no sharing the volume) or by proxying the request to olympia to return the file. Concurrently, the logic toggling whether we serve static assets from django is no longer based on either DEBUG or DEV_MODE but instead on a dedicated setting that is True outside of production. I've tested all four scenarios after this change and everything works. LMK if it fixes the issue for you.

I just tried this now (with a fresh gh pr checkout 22748) and it's the same - make up DEBUG= DOCKER_TARGET=production is missing static media. Is this working for you? You're literally typing the commands as in the testing instructions or doing something else?

@KevinMind KevinMind force-pushed the dev-mode branch 2 times, most recently from 8218b4e to a7565c3 Compare October 16, 2024 14:33
@KevinMind KevinMind marked this pull request as draft October 16, 2024 14:38
@KevinMind KevinMind force-pushed the dev-mode branch 2 times, most recently from e80d1b8 to 17960d9 Compare October 16, 2024 15:11
@KevinMind
Copy link
Contributor Author

Ok, I found some more interesting nuggets of info.

  1. Apparently django's ManifestStaticFilesStorage class changes the static file URLs depending on the DEBUG property.. it's not totally important, but an interesting outcome that seems kind of unintuitive in our world of DEV_MODE.
  2. Django has 2 static file serve views, and we actually need both. in prod mode, we concatenate and compress files and pop them in site-static but this is not available to the static file server without some custom configuration. Likewise the base level file server can serve files for dependencies so we need both.
  3. My testing instructions had an invalid portion DEBUG=1 will not set the value of debug correctly. better use DEBUG=True (which is also the default value)
  4. We had some dev related reloading logic depending on DEBUG that should depend on DEV_MODE

It should be working now @eviljeff please lmk.

@KevinMind KevinMind marked this pull request as ready for review October 17, 2024 08:28
settings_test.py Outdated Show resolved Hide resolved
src/olympia/lib/settings_base.py Outdated Show resolved Hide resolved
Copy link
Member

@eviljeff eviljeff left a comment

Choose a reason for hiding this comment

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

Testing results:
I expected to see django toolbar with make up DEBUG=True DOCKER_TARGET=production (because DEBUG=True) but it was missing.

Couldn't see any other problems 🤷

@KevinMind
Copy link
Contributor Author

Testing results: I expected to see django toolbar with make up DEBUG=True DOCKER_TARGET=production (because DEBUG=True) but it was missing.

Couldn't see any other problems 🤷

We cannot show that in prod mode because it is a dev dependency. We could consider moving that to prod dependency but would be a follow up.

@KevinMind KevinMind merged commit 93fb6c5 into master Oct 18, 2024
31 checks passed
@KevinMind KevinMind deleted the dev-mode branch October 18, 2024 15:39
@eviljeff
Copy link
Member

Testing results: I expected to see django toolbar with make up DEBUG=True DOCKER_TARGET=production (because DEBUG=True) but it was missing.
Couldn't see any other problems 🤷

We cannot show that in prod mode because it is a dev dependency. We could consider moving that to prod dependency but would be a follow up.

I'm not sure it should be a prod dependency, but in "expectations" you wrote "if debug is true, expect debug toolbar"

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.

[Bug]: production mode for addons-server local development is broken
3 participants