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

Get rid of pixl_config.yml #311

Merged
merged 24 commits into from
Feb 26, 2024
Merged

Get rid of pixl_config.yml #311

merged 24 commits into from
Feb 26, 2024

Conversation

milanmlft
Copy link
Member

@milanmlft milanmlft commented Feb 20, 2024

Attempt to reduce complexity by:

  • Removing pixl_config.yml and moving the useful bits to .env (which already largely had the information contained in pixl_config.yml)
  • Move the APIConfig class to cli._config
  • Reduce the use of the name config, to avoid confusion and in preparation of Support configuration for multiple projects #309

Motivation: in #309 we will add config.yml files to configure how projects should be run through the pipeline. The information contained in those config.yml files are completely different from what is in the current pixl_config.yml file, which may lead to some confusion. In addition, the information in pixl_config.yml is already encoded in the .env environment variables anyway.


To do

  • Check that system test still succeeds
  • Remove all references to pixl_config.yml in the docs
  • Document the appropriate way of configuring the EHR and Imaging APIs with the new env variables

@milanmlft milanmlft requested a review from a team February 20, 2024 19:29
cli/src/pixl_cli/_config.py Outdated Show resolved Hide resolved
cli/src/pixl_cli/_config.py Show resolved Hide resolved
cli/src/pixl_cli/_config.py Show resolved Hide resolved
@milanmlft milanmlft marked this pull request as ready for review February 21, 2024 12:06
Copy link
Contributor

@stefpiatek stefpiatek left a comment

Choose a reason for hiding this comment

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

Works on GAE. What's more fun is that if you're using the environment then it can be done from any directory if you've used pip install -e. Which I think is okay, or do we want it to read the config only from the directory that the cli is initiated in?

cli/src/pixl_cli/_config.py Outdated Show resolved Hide resolved
cli/src/pixl_cli/_config.py Show resolved Hide resolved
@milanmlft
Copy link
Member Author

milanmlft commented Feb 21, 2024

or do we want it to read the config only from the directory that the cli is initiated in?

I would say it's fine, not much chance that there would be any name clashes for these environment variables.

@stefpiatek
Copy link
Contributor

I would say it's fine, not much chance that there would be any name clashes for these environment variables.

Only risk is if you use the environment for the wrong project. e.g. I'm in dev but I use the test environment. Agree that I think its fine

@milanmlft milanmlft requested a review from a team February 23, 2024 14:09
@milanmlft milanmlft force-pushed the milanmlft/rm-pixl_config branch from 9529e56 to f32d463 Compare February 26, 2024 14:48
Copy link
Contributor

@stefpiatek stefpiatek left a comment

Choose a reason for hiding this comment

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

🎉

pixl_imaging/src/pixl_imaging/_orthanc.py Outdated Show resolved Hide resolved
@milanmlft milanmlft enabled auto-merge (squash) February 26, 2024 18:19
@milanmlft milanmlft merged commit 00ca23e into main Feb 26, 2024
8 checks passed
@milanmlft milanmlft deleted the milanmlft/rm-pixl_config branch February 26, 2024 18:28
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.

2 participants