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

Refactor app config flow (incl. dealing with flask env deprecation) #969

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

GustaafL
Copy link
Contributor

@GustaafL GustaafL commented Feb 1, 2024

Description

Refactor the flow in which the app config is loaded. This solves the issue where the documentation was unable to build by using a separate flow/function for the config loading for documentation, testing and (production, development, staging)

How to test

Execute the tests, try to build the documentation, starting flexmeasures with the environment variables coming from different locations.

Signed-off-by: GustaafL <guus@seita.nl>
Signed-off-by: F.N. Claessen <felix@seita.nl>
Signed-off-by: F.N. Claessen <felix@seita.nl>
@nhoening
Copy link
Contributor

nhoening commented Feb 1, 2024

Please, I need a description what this PR is doing. That's usually in the PR description.
There are two env settings handled now, and some refactoring.
What is the big picture?

Signed-off-by: F.N. Claessen <felix@seita.nl>
@Flix6x Flix6x mentioned this pull request Feb 2, 2024
@Flix6x
Copy link
Contributor

Flix6x commented Feb 2, 2024

We discussed splitting this PR into two:

  1. A separate PR now contains the fix that lets our documentation build pass again.
  2. The remainder of this PR would then be about refactoring how different environments are being set up in app.py, with the goal of creating more clarity (e.g. fewer if/else statements).

@GustaafL thanks for editing the PR description. Please also remove any subsections that just contain contents from the PR template, as they only distract.

Copy link
Contributor

@Flix6x Flix6x left a comment

Choose a reason for hiding this comment

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

I didn't really review this, but I just noticed these two things:

  1. See comment in check_app_env.
  2. It also looks like on of the checks is failing: https://github.com/FlexMeasures/flexmeasures/actions/runs/7745435994/job/21121419518?pr=969

"development",
"testing",
"staging",
"production",
):
Copy link
Contributor

Choose a reason for hiding this comment

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

The following print statement would need to be updated correspondingly, I suppose:

        print(
            f'Flexmeasures environment needs to be either "documentation", "development", "testing", "staging" or "production". It currently is "{env}".'
        )

@Flix6x
Copy link
Contributor

Flix6x commented Mar 11, 2024

@nhoening want to take over this PR, seeing as you authored most of our app config logic?

@nhoening nhoening self-assigned this Mar 15, 2024
@nhoening
Copy link
Contributor

This issue has as motivation "the issue where the documentation was unable to build".

That issue seems fine, looking at https://readthedocs.org/projects/flexmeasures/builds/

So I assume the motivation is now a refactoring for cleaner code. I'll have to review if I can wrap my head around this one of these days.

@Flix6x Flix6x changed the title Fix documentation issue after flask env deprecation Refactor app config flow (incl. dealing with flask env deprecation) Mar 15, 2024
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.

3 participants