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

Config from env vars must be normalized #7502

Merged
merged 7 commits into from May 24, 2023

Conversation

avdata99
Copy link
Member

CONFIG_FROM_ENV_VARS takes precedence over config file and extensions but those settings are not normalized

This is a work in progress.
Is this a bug? Am I missing something?

Features:

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

Please [X] all the boxes above that apply

@avdata99
Copy link
Member Author

Just a single test function from the test file updated in this PR is running 🙃

image

@smotornyuk
Copy link
Member

smotornyuk commented Mar 30, 2023

Example of normalizing a single option:

from ckan.common import config_declaration as cd

# get the Option object that contains normalization rules
option = cd['ckan.devserver.port']

# normalize value
assert option.normalize('5000') == 5000

# note: invalid values are not normalized and 
# do not produce any errors in this way
assert option.normalize('sf') == 'sf' 

@avdata99
Copy link
Member Author

avdata99 commented Apr 4, 2023

I think this is ready for review @smotornyuk

@smotornyuk smotornyuk self-assigned this Apr 6, 2023
@avdata99
Copy link
Member Author

avdata99 commented Apr 6, 2023

We decided in the tech meeting to stop giving precedente to CONFIG_FROM_ENV_VARS.
I am going to move the reading of the environment variables earlier in the function so that they are read first and normalized automatically.

@smotornyuk
Copy link
Member

@avdata99, would you happen to have time to update this PR on Monday? We will make a patch release soon, and it would be nice to include this fix. If not, I'll add a couple of commits to your PR.

@avdata99
Copy link
Member Author

@smotornyuk, sorry. No, I can't update this PR today. Please feel free to update the branch

@smotornyuk
Copy link
Member

Done

@smotornyuk smotornyuk merged commit c02f4d1 into ckan:master May 24, 2023
8 checks passed
@avdata99 avdata99 deleted the config_from_env_normalized branch May 30, 2023 12:40
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.

None yet

3 participants