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

Migrate Mquery to typed config library #324

Merged
merged 3 commits into from
Jan 24, 2023
Merged

Conversation

msm-code
Copy link
Contributor

@msm-code msm-code commented Jan 11, 2023

Your checklist for this pull request

  • I've read the contributing guideline.
  • I've tested my changes by building and running mquery, and testing changed functionality (if applicable)
  • I've added automated tests for my change (if applicable, optional)
  • I've updated documentation to reflect my change (if applicable)

This is a WIP draft PR for migration of mquery's config from current config mechanism (config.py) to typed-config.

It doesn't take configuration from redis to the config file yet. It's backwards compatible for docker and kubernetes users (i.e. ENV config didn't change). Despite this, it's still technically backward incompatible for users who use bare metal mquery and edited their config.py

Fixes #320
Fixes #318
Fixes #261

@msm-code msm-code changed the title WIP on typed config Migrate Mquery to typed config library Jan 12, 2023
@msm-code
Copy link
Contributor Author

msm-code commented Jan 12, 2023

Ok, i'm feeling lucky.

I'm submitting this PR for review and maybe merging. This changes the current mquery configuration mechanism from editing config.py file, to a "real" config file like this (mquery.ini):

[redis]
host=redis-server.example.com

[mquery]
backend=tcp://ursadb-server.example.com:9281
plugins=plugins.archive:GzipPlugin

This solution has some downsides:

  • For some of the users this is likely to break backward compatibility.
  • ...actually I think that's all.

Despite this, I like it because:

  • It's much cleaner and we save some boilerplate.
  • Installation procedure is easier thanks to good defaults (no need to copy and edit files before first use).
  • It's backward compatible for docker and kubernetes deployments
  • When it's not backward compatible, affected users will see merge conflicts and know that they need to migrate.
  • In the future I'll migrate mquery's auth config from redis to this file, bringing more redability and usability benefits.

I've also added a signifcant amount of documentation, to explain mquery config. But this chnage will have to be stressed in the release notes, so users that decide to upgrade know to be careful about that.

@msm-code msm-code marked this pull request as ready for review January 12, 2023 21:06
@msm-code msm-code requested a review from nazywam January 23, 2023 11:40
@@ -115,6 +109,9 @@ jobs:
run: docker-compose up --scale daemon=1 --build -d
- name: run e2e tests
run: docker run --net mquery_default -v $(readlink -f ./samples):/mnt/samples mquery_tests
- name: get run logs
if: always()
run: docker-compose logs
Copy link
Member

Choose a reason for hiding this comment

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

This wasn't just used for debugging and we're leaving this in, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This should've been a separate PR... But yeah, this is very much intended to stay. Without this, docker-compose logs are not available in the CI when something crashes.

docs/configuration.md Outdated Show resolved Hide resolved
docs/configuration.md Outdated Show resolved Hide resolved
docs/configuration.md Outdated Show resolved Hide resolved
docs/configuration.md Outdated Show resolved Hide resolved
docs/configuration.md Outdated Show resolved Hide resolved
docs/configuration.md Outdated Show resolved Hide resolved
docs/configuration.md Outdated Show resolved Hide resolved
docs/configuration.md Outdated Show resolved Hide resolved
docs/configuration.md Outdated Show resolved Hide resolved
@msm-code
Copy link
Contributor Author

msm-code commented Jan 24, 2023

Resolved discussions, squashed... Just realised that I should make it possible (or document) to configure mquery with a config-file even when docker-compose is used. Not sure how to do this correctly, will handle this tomorrow.

Edit: actually docker users should configure their instance with environment vars or mount the config themselves, so that's not applicable.

@msm-code msm-code merged commit 1e7b895 into master Jan 24, 2023
@msm-code msm-code deleted the feature/typed-config branch January 24, 2023 20:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants