-
Notifications
You must be signed in to change notification settings - Fork 2
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
PB-1440: allow setting SESSION_* settings via environment variables. #514
Conversation
cdd175e
to
b9fd173
Compare
SESSION_COOKIE_AGE = env('SESSION_COOKIE_AGE', default=60 * 60 * 24 * 7 * 2) | ||
SESSION_COOKIE_SAMESITE = env('SESSION_COOKIE_SAMESITE', default='Lax') | ||
SESSION_COOKIE_SECURE = env('SESSION_COOKIE_SECURE', default=False) |
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.
don't you want to set the defaults to what we want to have in prod?
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 would also leave the defaults like they were, the django defaults don't neccessarily make sense...
Defaulting SESSION_COOKIE_SECURE
to False
doesn't sound like a good idea.
Defaulting SESSION_COOKIE_SECURE
to Lax
when it already works with strict
also doesn't sound like a good idea.
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.
The current state is already 2 weeks, Lax and "insecure". This just makes the default explicit. We will change them via flags at Kubernetes level which is easier to rollback or tweak further than if we need to make a new release every time.
We can set the default to what we want in prod eventually but then it means setting the flags in dev, int, prod upfront while they have no actual effect, which I think would be more confusing. E.g. we could set SESSION_COOKIE_SECURE=True in settings_prod.py and False in {dev,int,prod}/kustomization.yaml. But then updating kustomization.yaml to set it to True won't actually do anything until the new STAC release is rolled out.
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.
Not sure about these defaults.
We want to tweak those but we don't need to hardcode the exact values for all environments immediately. Another change will update the values in DEV, INT then PROD. Afterwards we can consider setting these values by default in STAC itself. This also removes the specific values we set for the dev build.
``` app/config/settings_prod.py:355:0: C0301: Line too long (101/100) (line-too-long) ```
1393bfa
to
291f78d
Compare
We want to tweak those but we don't need to hardcode the exact values for all environments immediately. Another change will update the values in DEV, INT then PROD. Afterwards we can consider setting these values by default in STAC itself.
This also removes the specific values we set for the dev build.