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

fix: Sorts group mappings in ldap.toml by org_role #467

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

v0nNemizez
Copy link
Contributor

@v0nNemizez v0nNemizez commented Oct 15, 2024

Description

Due to the way Grafana reads group mappings, the mappings needs to be sorted so it reads them in this order: Admin->Editor -> Viewer.

Check List

  • A summary of changes made is included in the CHANGELOG under ## Unreleased
  • New functionality includes testing.
  • New functionality has been documented in the README if applicable.

@v0nNemizez v0nNemizez requested a review from a team as a code owner October 15, 2024 06:17
bmhughes
bmhughes previously approved these changes Oct 15, 2024
@bmhughes bmhughes added Bug Something isn't working Release: Patch Release to Chef Supermarket as a version patch when merged labels Oct 15, 2024
@bmhughes
Copy link
Contributor

Thanks for the PR @v0nNemizez!

Looks good in principal but it appears to have broken indempotency as the resource is converging on the second run, looks like load_current_value is returning the wrong existing config entry. Could be due to the the helper returning the first match but I'd expect it to work still so you'll need to do some debug there before we can merge this.

@bmhughes bmhughes added the Waiting on Contributor Awaiting on the person who raised this to update label Oct 15, 2024
@v0nNemizez
Copy link
Contributor Author

v0nNemizez commented Oct 16, 2024

what do you mean here? Im running this on a company test server running against the company AD with nested groups, and this works as intended and sovled our problems:)

@bmhughes
Copy link
Contributor

bmhughes commented Oct 16, 2024

If you look at the CI tests then you'll see that all the LDAP ones have failed as Kitchen has failed as the run shouldn't converge a second time but it has due to the ldap_group_mapping resource re-ordering the group_dn. It'll be something tied to how this PR changes the sort order.

https://github.com/sous-chefs/grafana/actions/runs/11340425253/job/31536848255?pr=467#step:4:2309

It may work but unfortunately it isn't right :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working Release: Patch Release to Chef Supermarket as a version patch when merged Waiting on Contributor Awaiting on the person who raised this to update
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants