Fix env config to variables

[paaragon]

Summary

This PR refactors the way we access to the environment variables centralizing them into one single class (one for the client and one for the gateway)

Details and comments

[avilches]

Finally, not sure if creating one single PR for the gateway and the client, which are two completely separate projects in the same repo, is the best idea. It makes the PRs unnecessary larger. Just to be clear, I'm not asking you to make another PR for the client, I'm just mentioning it for the next ones...

[avilches]

Do we really want this kind of file now, when we are using a Config class which is the only place that's actually using these constants? I mean, it makes sense to have all these constants and their defaults if we are going to use os.environ.get() calls anywhere in our code, so that if a variable name changes or a default is changed, we only have to do it in one place (the constant.py file). But if all accesses to these variables are made from Config, which is the new source of truth, what's the point of adding this level of indirection? I would just hardcode both the variables and the defaults within each method. That way, it's the only place where these values are hardcoded, and it's easier to see and modify.

[avilches]

The same approach with the settings.py class, why do we create a Config class when we only use it once during bootstrap to assign its values to a bunch of constants that need to be imported? I would remove the settings.py and only use the new Config class, which should be the de facto source of truth for the config/settings (config = settings, right?).

[Tansito]

In fact, if I remember correctly the proposal was done by @korgan00 but only for the client. I think Django is just managing this correctly. I wouldn't change the gateway.

[korgan00]

I proposed this for client but the reasons also apply for gateway I think. The main reason is not access to envars anywhere in the code having a default value in each call and unify everything in one class or somewhere, I dont know if there is a django way to improve this.

[Tansito]

But you access to the env_vars only in the settings.py. Through the code you use:

from django.conf import settings
....
settings.LIMITS_JOBS_PER_USER

I don't think there is nothing wrong with this approach. Or I don't think so at least.

This is what we were trying to fix in the client: https://github.com/Qiskit/qiskit-serverless/pull/1790/files#diff-fdc3aef5e1a595990db1b71f1ca195cbdfc589291d1f23c213c5fe735c8b0611L127

[avilches]

But you access to the env_vars only in the settings.py. Through the code you use:

from django.conf import settings
....
settings.LIMITS_JOBS_PER_USER

I don't think there is nothing wrong with this approach. Or I don't think so at least.

This is what we were trying to fix in the client: https://github.com/Qiskit/qiskit-serverless/pull/1790/files#diff-fdc3aef5e1a595990db1b71f1ca195cbdfc589291d1f23c213c5fe735c8b0611L127

The main goal of the issue is to define a single source of truth (settings, Config, or whatever) instead of reading environment variables scattered throughout the codebase. To achieve this, I'm fine with having a Config class with methods, or just a bunch of constants in the settings.py. But we can't have it both at the same time...

settings.py

CUSTOM_IMAGE_PACKAGE_NAME = Config.custom_image_package_name()

which calls to config.py:

def custom_image_package_name(cls) -> str:
        """Get custom image package name."""
        return os.environ.get(
            CUSTOM_IMAGE_PACKAGE_NAME, CUSTOM_IMAGE_PACKAGE_NAME_DEFAULT
        )

which requires constants.py

CUSTOM_IMAGE_PACKAGE_NAME = "CUSTOM_IMAGE_PACKAGE_NAME"
CUSTOM_IMAGE_PACKAGE_NAME_DEFAULT = "CUSTOM_IMAGE_PACKAGE_NAME_DEFAULT"

I suggest having only settings, only Config, but no both. And remove the constants. That will make easier for us: just one single place to use and one single place to add/update variables.

Regarding constants in settings vs class Config, I think class is superior because:

• It requires importing the Class, not every constant needed in your class.
• The Config class has methods that are executed every time they are needed, not only in the bootstrap. We can play with this: read from different sources (database, LaunchDarkly, AppConfig), cache them or do whatever we need.

[Tansito]

For the purpose of this PR as it was created focusing on the client my proposal is to create an issue for the gateway and have the final discussion there. WDYT @avilches , @korgan00 , @paaragon . I think everyone agrees at least that this PR should be simplified and contains only the changes in the client.

[paaragon]

Hi @Tansito @avilches @korgan00

I have removed the changes in the gateway

[korgan00]

Agree with @avilches . We can just remove the constants file now and use literals in the Config file.

[paaragon]

Agree with @avilches . We can just remove the constants file now and use literals in the Config file.

Ok, this is what I did: I have changed the config file to use literals instead of constants. Also, I have removed from the constants.py file all the variables that weren't being used

[avilches]

Why was the "provider" part removed from the gateway host, token and version? I mean, why not gateway_provider_token, gateway_provider_version and gateway_provider_host instead... is the "provider" word redundant in the gateway?

[avilches]

Why have two methods (where one calls the other) instead of just one?

[avilches]

I think these variables should be removed too. If the codebase is still using them, then their usage should be done through the new Config class.

[avilches]

These two config values have the same environment variable REQUESTS_TIMEOUT_OVERRIDE

[avilches]

I should be ibm_serverless_host_url_override

[avilches]

Suggested change

	host: host of gateway. Optional. It uses IBM_SERVERLESS_HOST_URL_OVERRIDE env var or IBM host

Wrong environment variable, my bad... 0294630#diff-fdc3aef5e1a595990db1b71f1ca195cbdfc589291d1f23c213c5fe735c8b0611R112

[avilches]

Check comments