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

SECRET_KEY: fall back to beaker.session.secret #7853

Merged
merged 7 commits into from
Sep 9, 2024
Merged

Conversation

wardi
Copy link
Contributor

@wardi wardi commented Oct 5, 2023

Proposed fixes:

  • default secret_key to beaker.session.secret for easier config migration/sharing configurations between ckan versions
  • a small config documentation fix

Features:

  • includes tests covering changes
  • includes updated documentation
  • includes user-visible changes
  • includes API changes
  • includes bugfix for possible backport

@wardi
Copy link
Contributor Author

wardi commented Oct 5, 2023

@amercader interested in your thoughts on this one. If the uppercase SECRET_KEY is a flask convention should we change envvars so it can generate this option instead?

@amercader
Copy link
Member

I think we should align with Flask and keep the upper case SECRET_KEY and make ckanext-envvar catch up and support uppercase config options as we now have plenty of those declared in the config declaration (coming from Flask-login, WTForms, etc) which would be great to support.

I made a first try in ckanext-envvars, using the config declaration if present (ie CKAN>=2.10) to keep those keys declared as they are when parsing the env vars. This works well in CKAN 2.11 (master) but not in CKAN 2.10. The problem is that SECRET_KEY is declared as internal: true in CKAN 2.10 which I think prevents it from being returned when doing config_declaration.iter_options().

ckan/ckanext-envvars#9

I'll keep investigating but does this seem like a good approach to explore?

@wardi
Copy link
Contributor Author

wardi commented Oct 6, 2023

@amercader ckan/ckanext-envvars#9 looks good to me.

I'll remove the lowercase secret_key part of this PR

This reverts commit 4160fac
but keeps some unrelated fixes
@wardi wardi changed the title use lowercase secret_key SECRET_KEY: fall back to beaker.session.secret Oct 6, 2023
@amercader
Copy link
Member

ckanext-envvars 0.0.4 supports upper case config options

@wardi
Copy link
Contributor Author

wardi commented Nov 2, 2023

@amercader I merged #7884 so we can see the tests passing, once we merge that to master this will be easier to review

@amercader amercader merged commit 03c1855 into master Sep 9, 2024
9 checks passed
@amercader amercader deleted the quiet-secrets branch September 9, 2024 13:41
@ckanbot
Copy link

ckanbot commented Sep 9, 2024

Backport failed for dev-v2.11, because it was unable to cherry-pick the commit(s).

Please cherry-pick the changes locally and resolve any conflicts.

git fetch origin dev-v2.11
git worktree add -d .worktree/backport-7853-to-dev-v2.11 origin/dev-v2.11
cd .worktree/backport-7853-to-dev-v2.11
git switch --create backport-7853-to-dev-v2.11
git cherry-pick -x 4160facd55bedf321b0f32d9b73f57f2aea8730a 1ab96909f6b04f0d90a8bd7ed2758f315dc97b12 1cd72262274895e08a0d835e79f391b3712dddc2

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.

3 participants