LDAP Refactoring to support syncronizing more than one user at a time. #16705
Conversation
|
Looks like a good start! Try to make this feature into small mergable PRs (like this), so let's try to get this refactor into a mergable state :) |
markelog
force-pushed
the
active-ldap
branch
2 times, most recently
from
084ba53
to
33f829d
Compare
There was a problem hiding this comment.
Great job!
I'd like to see the things I pointed to in the review fixed so that we can merge this. Transactions, tests, docs and better error handling can come in the next PR. If we can get this merged it makes it easier to keep working on the parts in Enterprise.
pkg/services/ldap/settings.go
Outdated
| } | ||
|
|
||
| ldapLogger.Info("Ldap enabled, reading config file", "file", setting.LdapConfigFile) | ||
| getConfig(setting.LdapConfigFile, config) |
There was a problem hiding this comment.
We should change the logic here so that the package-local config var is not set to point to the real config until it is fully read. There is the potential for a race condition here.
There was a problem hiding this comment.
How should we mitigate that?
There was a problem hiding this comment.
Actually, my bad, there's no race condition in the code. There is also no caching of the config variable as that is never assigned. I thought we used the package config variable to readthe config, but that is overshadowed by the func local variable config. The only time we use the package scoped config is when we check if it has already been set.
My suggestion would be to just use another named config varaibled in the local scope to read the config and then assign a pointer to that var to the packaged scoped one afterwards, that should make the code work as intended and makes it a little bit easier to understand as well.
Wanted to propose just that! Haha. I was thinking to finish up with the refactoring of the main parts (which are almost done in my local branch) and then I think it should be ready. Everything else should follow in a way that wouldn't affect other parts of the system. Except of the further refactoring of the newly added ldap service. The only other I thing I was thinking to do beside that - is to recheck all cases where changed parts are used. Like those aforementioned posix stuff... might be a challenge |
xlson
changed the title
|
Might want to rename |
Oh yeah-yeah! I was thinking to do that in the refactoring pr, certainly wouldn't want to leave |
* Move ldap configuration read to special function * Introduce cron setting (no docs for it yet, pending approval)
This is very early preliminary implementation of active sync. There is only one thing that's going right for this code - it works. Aside from that, there is no tests, error handling, docs, transactions, it's very much duplicative and etc. But this is the overall direction with architecture I'm going for
* grafana/master: LDAP Refactoring to support syncronizing more than one user at a time. (grafana#16705) build: removes unused vendored files (fix/explore): remove vertical-align looks better for long logs (grafana#16736) Chore: bump jQuery to 3.4.0 in grafana/ui (grafana#16781) Devenv: Updated home dashboard and added new influxdb test dashboard Chore: Lowered implicit anys limit to 5946 RefreshPicker: minor design update (grafana#16774)
* grafana/master: build: restore postgres integration tests (grafana#16801) docs: explain correct access control model of GCS buckets (grafana#16792) Chore: Fixed no implicit any Typescript errors (grafana#16799) Feature: introduce LdapActiveSyncEnabled setting (grafana#16787) Plugins: ReactPanelPlugin to VizPanelPlugin (grafana#16779) UX: Improve Grafana usage for smaller screens (grafana#16783) ThresholdEditor: Minor style fix for smaller screens (grafana#16791) Build: Use isolated modules for ts-jest (grafana#16786) LDAP Refactoring to support syncronizing more than one user at a time. (grafana#16705) build: removes unused vendored files (fix/explore): remove vertical-align looks better for long logs (grafana#16736) Chore: bump jQuery to 3.4.0 in grafana/ui (grafana#16781) Devenv: Updated home dashboard and added new influxdb test dashboard Chore: Lowered implicit anys limit to 5946 RefreshPicker: minor design update (grafana#16774) Streaming: support streaming and a javascript test datasource (grafana#16729)
* grafana/master: build: restore postgres integration tests (grafana#16801) docs: explain correct access control model of GCS buckets (grafana#16792) Chore: Fixed no implicit any Typescript errors (grafana#16799) Feature: introduce LdapActiveSyncEnabled setting (grafana#16787) Plugins: ReactPanelPlugin to VizPanelPlugin (grafana#16779) UX: Improve Grafana usage for smaller screens (grafana#16783) ThresholdEditor: Minor style fix for smaller screens (grafana#16791) Build: Use isolated modules for ts-jest (grafana#16786) LDAP Refactoring to support syncronizing more than one user at a time. (grafana#16705) build: removes unused vendored files (fix/explore): remove vertical-align looks better for long logs (grafana#16736) Chore: bump jQuery to 3.4.0 in grafana/ui (grafana#16781) Devenv: Updated home dashboard and added new influxdb test dashboard Chore: Lowered implicit anys limit to 5946 RefreshPicker: minor design update (grafana#16774)
…MetricPanelCtrl * grafana/master: build: restore postgres integration tests (grafana#16801) docs: explain correct access control model of GCS buckets (grafana#16792) Chore: Fixed no implicit any Typescript errors (grafana#16799) Feature: introduce LdapActiveSyncEnabled setting (grafana#16787) Plugins: ReactPanelPlugin to VizPanelPlugin (grafana#16779) UX: Improve Grafana usage for smaller screens (grafana#16783) ThresholdEditor: Minor style fix for smaller screens (grafana#16791) Build: Use isolated modules for ts-jest (grafana#16786) LDAP Refactoring to support syncronizing more than one user at a time. (grafana#16705) build: removes unused vendored files (fix/explore): remove vertical-align looks better for long logs (grafana#16736) Chore: bump jQuery to 3.4.0 in grafana/ui (grafana#16781) Devenv: Updated home dashboard and added new influxdb test dashboard Chore: Lowered implicit anys limit to 5946 RefreshPicker: minor design update (grafana#16774)
* grafana/master: build: restore postgres integration tests (grafana#16801) docs: explain correct access control model of GCS buckets (grafana#16792) Chore: Fixed no implicit any Typescript errors (grafana#16799) Feature: introduce LdapActiveSyncEnabled setting (grafana#16787) Plugins: ReactPanelPlugin to VizPanelPlugin (grafana#16779) UX: Improve Grafana usage for smaller screens (grafana#16783) ThresholdEditor: Minor style fix for smaller screens (grafana#16791) Build: Use isolated modules for ts-jest (grafana#16786) LDAP Refactoring to support syncronizing more than one user at a time. (grafana#16705) build: removes unused vendored files (fix/explore): remove vertical-align looks better for long logs (grafana#16736) Chore: bump jQuery to 3.4.0 in grafana/ui (grafana#16781) Devenv: Updated home dashboard and added new influxdb test dashboard Chore: Lowered implicit anys limit to 5946 RefreshPicker: minor design update (grafana#16774)
* master: (470 commits) removes gopkg.lock from root folder Explore: Support user timezone (grafana#16469) Plugins: rename vizPlugin to panelPlugin (grafana#16802) Plugins: move app/feature/plugin properties into PluginMeta (grafana#16809) Plugins: move PanelPluginMeta to grafana/ui (grafana#16804) Plugins: move datasource specific meta out of the main meta type (grafana#16803) updates changelog for 6.1.6 Fix: Fetch histogram series from other api route (grafana#16768) phantomjs: set web-security to true Chore: Lowered implicit anys limit to 5668 build: restore postgres integration tests (grafana#16801) docs: explain correct access control model of GCS buckets (grafana#16792) Chore: Fixed no implicit any Typescript errors (grafana#16799) Feature: introduce LdapActiveSyncEnabled setting (grafana#16787) Plugins: ReactPanelPlugin to VizPanelPlugin (grafana#16779) UX: Improve Grafana usage for smaller screens (grafana#16783) ThresholdEditor: Minor style fix for smaller screens (grafana#16791) Build: Use isolated modules for ts-jest (grafana#16786) LDAP Refactoring to support syncronizing more than one user at a time. (grafana#16705) build: removes unused vendored files ...
* grafana/master: build: restore postgres integration tests (grafana#16801) docs: explain correct access control model of GCS buckets (grafana#16792) Chore: Fixed no implicit any Typescript errors (grafana#16799) Feature: introduce LdapActiveSyncEnabled setting (grafana#16787) Plugins: ReactPanelPlugin to VizPanelPlugin (grafana#16779) UX: Improve Grafana usage for smaller screens (grafana#16783) ThresholdEditor: Minor style fix for smaller screens (grafana#16791) Build: Use isolated modules for ts-jest (grafana#16786) LDAP Refactoring to support syncronizing more than one user at a time. (grafana#16705) build: removes unused vendored files (fix/explore): remove vertical-align looks better for long logs (grafana#16736) Chore: bump jQuery to 3.4.0 in grafana/ui (grafana#16781) Devenv: Updated home dashboard and added new influxdb test dashboard Chore: Lowered implicit anys limit to 5946 RefreshPicker: minor design update (grafana#16774)
This is very early preliminary implementation of active sync.
There is only one thing that's going right for this code - it works.
Aside from that, there is no tests, error handling, docs, transactions,
it's very much duplicative and etc.
But this is the overall direction with architecture I'm going for.
Incorporates #16673