Build #581 failed (commit: 505601d85fb61f3677094a22b5cd6abd2b783110).
Rebase or make new commits to rebuild.

After the change, we have three config files:
1. conf/base_config: BaseConfiguration, the common configurations are here
2. conf/web_config: we can add frontend configurations here
3. conf/workers_config: we can add workers configurations here

when 'fedmsg-hub*' or 'celery' is found in command line, we load configurations from workers_config and create the config object, otherwise we load configurations from web_config.

One thing I'm not sure is should we handle local module build differently, should we load different configuration from another file for local module build?

The c3i CI test failed due to rpmbuild failure because mbs config file is changed in this commit.

The issue this pr is trying to fix is for the migration to run backend in Celery workers, where no fedmsg-hub appears in command line. This condition should be needed to be reconsidered.

The issue this pr is trying to fix is for the migration to run backend in Celery workers, where no fedmsg-hub appears in command line. This condition should be needed to be reconsidered.

Once the migration is finished (the celery workers can run), this will be updated.

You could also just do arg.startswith(("fedmsg-hub", "celery")).

You could also just do arg.startswith(("fedmsg-hub", "celery")).

ok, it doesn't hurt to have it ready first :)

Patch is updated with one line change per comment.

This looks good to me, let's wait for @mprahl.

Can we decouple init_config with flask here? I'm thinking init_config should be told from where and which config section to load config. For the former, init_config could accept an argument for values web and backend. For the latter, init_config just checks the supported environment variables from the 'environ', which should be the os.environ, or the merge result of app.request.environ and os.environ if loading for web, and finally passed into function.

On the other hand, does Flask app.request.environ contain all values from os.environ when start frontend locally? If yes, init_config can be simplified too much.

Build #583 failed (commit: 7a1801759fcc495eb10dff482445511a301e15f5).
Rebase or make new commits to rebuild.

Can we decouple init_config with flask here?

We can't simply remove flask_app here, beside of creating the config object, we also configure flask_app (while running as frontend) with:

flask_app.config.from_object(config_section_obj)

A doable way is to rename init_config to load_config and make the function just load config from specific config file. Then, in module_build_service/__init__.py, initialize the global variable conf and config flask app app.config.from_object(config_section_obj). Like this:

config_section_obj = load_config(...)
conf = Config(config_section_obj)
app.config.from_object(config_section_obj)

A doable way is to rename init_config to load_config and make the function just load config from specific config file. Then, in module_build_service/init.py, initialize the global variable conf and config flask app app.config.from_object(config_section_obj).

I don't see any benefit of this and I'd prefer not to import any other internal modules (config to have Config here) in __init__.py as possible as we can.

I think the BaseConfiguration class should be removed. All default values that are MBS specific should be present in module_build_service/config.py. If they are not, that is a bug. Any Flask configuration should be in the new WebConfiguration class. The only exception I can think of is sqlalchemy_database_uri, which needs to be in both the WebConfiguration and WorkersConfiguration class.

I know it's confusing, but we started out with just configuration classes, and then module_build_service/config.py was introduced and we never cleaned this up.

Could you set this as a default value in module_build_service/config.py instead? I'm not sure why it's not set there.

Could you please remove this? This is the default in module_build_service/config.py.

Could you please remove this? This is the default in module_build_service/config.py.

Could you please remove this? This is the default in module_build_service/config.py.

Could you please remove this? This is the default in module_build_service/config.py.

Could you please remove this? This is the default in module_build_service/config.py.

Could you please remove this? This is the default in module_build_service/config.py.

@qwan I started identifying the configuration values that are just setting the default value from module_build_service/config.py, but there are too many to comment on each one. Could you please clean that up?

@qwan this is optional, but I think the cleanest approach would be to use the Flask application factory methodology as described in https://flask.palletsprojects.com/en/1.1.x/patterns/appfactories/.
Our other projects such as Cachito, Greenwave, and WaiverDB all use this approach.

Then in the create_app function, it would configure Flask from the configuration created by init_config.

Then MBS could use a global configuration object for the time being for both the frontend and backend, but the frontend only configures Flask and the backend would configure Celery.

Optional: You could use class inheritance where you would have Config as the base class and then you could have a class called WebConfig (APIConfig works too) and BackendConfig (WorkerConfig works too) that both inherit from the Config class. Then the constructors for the subclasses would handle things differently. It might be cleaner over having a bunch of if statements.

Build #595 failed (commit: 15b5bc5c8a68ae53b53102642aeac47bd948486d).
Rebase or make new commits to rebuild.

Updated patch per comments:
1. Removed BaseConfiguration and base_config.py file.
2. Set default value of BUILD_LOGS_DIR in config.py.
3. Removed all settings with defaults values in config.py from config files.

Hi @mprahl , for the 2 optional changes you suggested, they're not addressed in this update:
1. Creating configuration with init_config and then create_app: currently init_config has some code of checking flask app envs to determine the config file and config section, we need pass flask app to init_config if we still need that part of code, and I'm not sure whether it's ok to remove them. I can open another PR to do it (the factory methodology) if we can drop that part of code in init_config.
2. Split up Config: to split the Config, we need to check all the 101 options to know which are frontend only, backend only or shared by both, it need some time to figure out, I've created an internal issue (FACTORY-5533 ) for it.

Build #596 failed (commit: 932f6c0737f991f5dd4385df038ebb67c9898fee).
Rebase or make new commits to rebuild.

Rebased on top of latest v3 branch.
Please ignore the ci failures:
1. 'c3i-jenkins' failed due to rpmbuild error because we changed the config file.
2. 'jenkins' failed because it's trying to merge the patch against master rather than v3.

Tests have been ran with both py2 and py3 locally, and passed.

Updated patch per comments:
1. Removed BaseConfiguration and base_config.py file.
2. Set default value of BUILD_LOGS_DIR in config.py.
3. Removed all settings with defaults values in config.py from config files.
Hi @mprahl , for the 2 optional changes you suggested, they're not addressed in this update:
1. Creating configuration with init_config and then create_app: currently init_config has some code of checking flask app envs to determine the config file and config section, we need pass flask app to init_config if we still need that part of code, and I'm not sure whether it's ok to remove them. I can open another PR to do it (the factory methodology) if we can drop that part of code in init_config.
2. Split up Config: to split the Config, we need to check all the 101 options to know which are frontend only, backend only or shared by both, it need some time to figure out, I've created an internal issue (FACTORY-5533 ) for it.

@qwan I was just suggesting to have two classes but both inherit the current Config class. This has two benefits. The first being that we can have separate constructors instead of an init_config method with if conditions for both as mentioned previously. The second is that any new configuration values that are specific to the frontend or backend can go in the correct configuration class. In particular, some of the Celery configuration is going to be worker specific.

Actually splitting that configuration up can be left in the backlog. I might take it up if I have spare cycles this sprint.

@qwan I was just suggesting to have two classes but both inherit the current Config class.

Hi @mprahl , I see, thanks for the comments, I updated the commit to address that, could you take a look again? unittests passed locally with both py2 and py3.

Build #601 failed (commit: b811654579d580551827770d99ae809f53bca91e).
Rebase or make new commits to rebuild.

@qwan what do you think about making this a classmethod so that it can be overridden and has a common class API? BackendConfig.init_config If you can, some of the base logic could be moved to the base class to avoid code duplication (in its own init_config method or helper methods that the derived classes can call), but I think some duplication will be necessary.

@qwan what do you think about making this a classmethod so that it can be overridden and has a common class API?

Hi @mprahl , can we do that when we have the chance to split MBS to sub components/packages (there is already an internal issue to track that)? As we have frontend config checking flask app environs, but backend doesn't, I feel like constructing some common APIs for both frontend and backend doesn't help much on making the code cleaner.
Or if we can remove the code of checking flask app environs for frontend config, it would be much easier, I don't know the background and importance of having such code, so didn't make such change.

You also need to check if this is a local build by checking if build_module_locally is in sys.argv.

Optional: It'd be nice to make this an instance variable instead of a class variable. Then you could use normal inheritance in WebConfig and BackendConfig instead of copying the static variable to the subclasses.

@qwan this is good enough to merge after addressing my comments.

You also need to check if this is a local build by checking if build_module_locally is in sys.argv.

Done

Optional: It'd be nice to make this an instance variable instead of a class variable. Then you could use normal inheritance in WebConfig and BackendConfig instead of copying the static variable to the subclasses.

Done, added __init__ in subclasses to update _defaults rather than copy the default settings as class variable.

Build #609 failed (commit: f47c1342ee6747203e61f62f4b93e9e146538463).
Rebase or make new commits to rebuild.

+1