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

Fixed validate settings #31

Merged
merged 3 commits into from
Nov 6, 2021
Merged

Fixed validate settings #31

merged 3 commits into from
Nov 6, 2021

Conversation

abhi1693
Copy link
Contributor

@abhi1693 abhi1693 commented Nov 2, 2021

I had the MINIO_USE_HTTPS=False in my settings and the library failed to start as validate_settings requires USE_HTTPS as True which should not be enforced on the user.

I have removed it from mandatory settings and set it's default value as False

@abhi1693
Copy link
Contributor Author

abhi1693 commented Nov 5, 2021

@theriverman Can you please validate this?

@theriverman
Copy link
Owner

theriverman commented Nov 6, 2021

Hi, @abhi1693

Thanks for the PR, this is indeed a bug due to my poor solution.
However, I'm not perfectly happy with this proposed change:

self.__MINIO_USE_HTTPS: bool = get_setting("MINIO_USE_HTTPS", False)

I haven't set default values for these settings parameters because I would like to enforce the user to explicitly configure it.
We can't assume whether their used MinIO instance will be http or https, so the best option is keeping this responsibility in the user's hand.

I propose to go with something like this

mandatory_parameters = (self.__MINIO_ENDPOINT, self.__MINIO_ACCESS_KEY, self.__MINIO_SECRET_KEY)
if any([bool(x) is False for x in mandatory_parameters]) or (get_setting("MINIO_USE_HTTPS") is None):
    raise ConfigurationError(
        "A mandatory parameter (ENDPOINT, ACCESS_KEY, SECRET_KEY or USE_HTTP) hasn't been configured properly"
    )

@theriverman theriverman changed the base branch from master to develop November 6, 2021 15:15
@theriverman theriverman merged commit d889d66 into theriverman:develop Nov 6, 2021
theriverman added a commit that referenced this pull request Nov 6, 2021
The value detection of "MINIO_USE_HTTPS" was faulty. False values were treated incorrectly.
theriverman added a commit that referenced this pull request Nov 6, 2021
Changes:
  * The value detection of "MINIO_USE_HTTPS" was faulty. False values were treated incorrectly.
  * Updated the docstring
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.

None yet

2 participants