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

Add option to disable admin password #261

Conversation

thegreatsunra
Copy link

@thegreatsunra thegreatsunra commented Jun 5, 2023

What It Does

  • Adds an option disablePassword to values.yaml that, if true, will set the Pi-hole admin password to "" thus disabling it.

Related Issue

Resolves: #251

@thegreatsunra
Copy link
Author

Noted the failure in the latest workflow run. Merged in the latest on upstream to bring in the latest up-to-date version.

@MoJo2600
Copy link
Owner

MoJo2600 commented Jun 28, 2023

Thank you for your contribution. I tried to make some changes to your PR but I'm not allowed to push to your repository. I put my changes now here: https://github.com/MoJo2600/pihole-kubernetes/tree/feat/no-admin-password

My idea was to not have to introduce a new parameter to the values.yml. Instead it would be possible to have three different ways to set a password:

  • adminPassword: - Without a value the password would be automatically generated which should be the default - I updated the NOTES.txt to show how to retrieve the password after installation
  • adminPassword: "" - If you provide an empty string, no password is set at all
  • adminPassword: "yourpassword" - yourpassword will be set as admin password

Question is, if this is a breaking change to the chart. Because then this change should be added to v3 of this chart. There some more breaking changes that would introduce breaking changes. So this pr would have to wait for the release of v3.

@thegreatsunra
Copy link
Author

Thank you for the feedback!

I tried to make some changes to your PR but I'm not allowed to push to your repository.

Ahh, shoot. I thought by ticking Allowing edits by maintainers you'd be able to commit directly to the branch, but I suppose not. I've added you as a Collaborator, which might address the issue.

My idea was to not have to introduce a new parameter to the values.yml

Nice. Your approach is cleaner and would reduce config sprawl, so I'm on board. To your point, it would change the existing behavior of the chart, whereas adding a new disablePassword parameter would make it an opt-in change while preserving existing behavior.

If we change the behavior by implementing the approach you suggest, then yeah, this change should roll out with v3.

@MoJo2600
Copy link
Owner

There was a second approach to add a feature to disable the admin password so I merged it. See: #274 - So I'm closing your pull request. Thank you nonetheless

@MoJo2600 MoJo2600 closed this Jan 24, 2024
@thegreatsunra
Copy link
Author

Good solution! Glad to see this implemented, and thank you! 😊

@thegreatsunra thegreatsunra deleted the 251-add-option-to-disable-admin-password branch January 24, 2024 20:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature request: add option to disable admin password
2 participants