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.

Sign up for GitHub

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

Jump to bottom

Clean out Mongo unused settings #21

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Clean out Mongo unused settings #21

wants to merge 3 commits into from

Conversation

WinnyTroy
Copy link
Contributor

Have mongo settings set to None to avoid attempts to connect to Mongo DB that we do not have for our KPI installation.

Closes #18

@WinnyTroy WinnyTroy requested a review from DavisRayM March 10, 2021 07:25
DavisRayM
DavisRayM suggested changes Mar 10, 2021
View reviewed changes
templates/kpi/settings.py.j2
Comment on lines +768 to +774
MONGO_CONNECTION = None
MONGO_DB = None
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we make the two values configurable via the vars with the default being None... Would make it easier in the future to utilize this functionality if the need arises

...
{% if enable_mongo_connection %}
MONGO_CONNECTION = MongoClient({{ mongo_connection_ url }}, .....
{% endif %}
...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good.
Made this change

@WinnyTroy WinnyTroy force-pushed the mongo_unused_settings branch from 51611bf to b80a8e0 Compare March 10, 2021 08:28
DavisRayM
DavisRayM reviewed Mar 10, 2021
View reviewed changes
Copy link
Contributor

@DavisRayM DavisRayM left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Since we've already made the ability to configure whether the mongo connection should be enabled it might be worthwhile to make this configurable too

@WinnyTroy
Copy link
Contributor Author

Since we've already made the ability to configure whether the mongo connection should be enabled it might be worthwhile to make this configurable too

I dont believe this would be necessary, if you see here, we first confirm that the Mongo DB user and password are present to create the connection.
Since we dont define those here, I believe this wont require a enable hook

@DavisRayM
Copy link
Contributor

Since we've already made the ability to configure whether the mongo connection should be enabled it might be worthwhile to make this configurable too

I dont believe this would be necessary, if you see here, we first confirm that the Mongo DB user and password are present to create the connection.
Since we dont define those here, I believe this wont require a enable hook

Mhhm yea.... makes me wonder should those be gotten from the environment ? Seems weird for an ansible role... I'd expect it to be configurable from the playbook instead of having the user export those environment variables in later steps

@WinnyTroy WinnyTroy force-pushed the mongo_unused_settings branch from b80a8e0 to f5249bc Compare March 11, 2021 07:12
DavisRayM
DavisRayM approved these changes Mar 11, 2021
View reviewed changes
Copy link
Contributor

@DavisRayM DavisRayM left a comment

Choose a reason for hiding this comment

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

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@DavisRayM DavisRayM DavisRayM approved these changes

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Set Mongo var in local_settings to none
2 participants
@WinnyTroy @DavisRayM