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

Changed Templates to use the new remoteAuth.backends array, instead of .backend #172

Merged
merged 4 commits into from
May 8, 2024

Conversation

Nabsku
Copy link
Contributor

@Nabsku Nabsku commented Dec 11, 2023

First helm related commit, so be gentle. :p

In the new 5.0 release, remoteAuth.backend is deprecated and you have to specify both remoteAuth.backend and remoteAuth.backends to get a working LDAP configuration.

This PR aims to fix that by removing all references to remoteAuth.backend.

@bootc
Copy link
Member

bootc commented Dec 11, 2023

Oh, thanks for that!

@bootc bootc self-assigned this Dec 11, 2023
@bootc bootc added the bug Something isn't working label Dec 11, 2023
@Danielp93
Copy link

I was running into LDAP configuration / Authentication issues after the upgrade to chart 5.0.0. Traced it back to this exact same issue and while working on the same fix I noticed this Pull Request. Thanks for this 👍

Tested; This fixes upgrade from 4.1.1 to 5.0.0 with LDAP remoteAuth backend.

@RangerRick
Copy link
Contributor

@Nabsku We've just done some rearranging of the repo to align it better with common practice for Helm charts. Mind rebasing this to match the new paths?

Also, it looks like maybe we need to add discussion of this change to the README.md so folks who upgrade from 4.x know the formatting is different. (Or add some shunts for backwards-compatibility so the old singular version still works. That said, the upcoming 5.x chart seems like the time to make breaking changes.)

@RangerRick RangerRick added the more info More information required from the reporter label May 2, 2024
@Nabsku
Copy link
Contributor Author

Nabsku commented May 6, 2024

@Nabsku We've just done some rearranging of the repo to align it better with common practice for Helm charts. Mind rebasing this to match the new paths?

Also, it looks like maybe we need to add discussion of this change to the README.md so folks who upgrade from 4.x know the formatting is different. (Or add some shunts for backwards-compatibility so the old singular version still works. That said, the upcoming 5.x chart seems like the time to make breaking changes.)

I've rebased my branch, thanks for the hint!

Also, regarding the discussion part, I believe that is how I found out about the missing changes in the helm chart - thus contributing this. We might need to make it more prominent but it's there. :)

@RangerRick
Copy link
Contributor

I will blame this on reading through too many issues at once. I swear I did a git grep and didn't see the references, but I was clearly blind. 😅

@RangerRick RangerRick removed the more info More information required from the reporter label May 6, 2024
@RangerRick
Copy link
Contributor

@Nabsku FYI, I've pushed a couple of updates to your branch so the tests would get farther, but I am flummoxed by the linting error now. The indentation seems right, and there aren't any extra spaces anywhere, and your changes don't touch that line as far as I can tell.

@Nabsku
Copy link
Contributor Author

Nabsku commented May 8, 2024

@Nabsku FYI, I've pushed a couple of updates to your branch so the tests would get farther, but I am flummoxed by the linting error now. The indentation seems right, and there aren't any extra spaces anywhere, and your changes don't touch that line as far as I can tell.

Can you run the checks again? I believe it might've been a mistake on my side but I am pretty sure it worked before.. Running it locally works fine now :)

Copy link
Member

@bootc bootc left a comment

Choose a reason for hiding this comment

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

All looks pretty sane to me. Do we need any changes to the README.md to follow up from this?

@bootc
Copy link
Member

bootc commented May 8, 2024

Do we need any changes to the README.md to follow up from this?

I clearly failed to read the rest of the discussion in the MR. I think we're good.

Comment on lines +78 to +79
{{- end }}
{{- end }}
Copy link
Contributor

Choose a reason for hiding this comment

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

those dang dashes 😅

@RangerRick RangerRick merged commit bf14c17 into netbox-community:develop May 8, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants