MoJo2600 / pihole-kubernetes

PiHole on kubernetes
493 stars 171 forks source link

Add option to disable admin password #261

Closed thegreatsunra closed 5 months ago

thegreatsunra commented 1 year ago

What It Does

Related Issue

Resolves: https://github.com/MoJo2600/pihole-kubernetes/issues/251

thegreatsunra commented 1 year ago

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

MoJo2600 commented 1 year ago

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:

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 commented 1 year ago

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 commented 5 months ago

There was a second approach to add a feature to disable the admin password so I merged it. See: https://github.com/MoJo2600/pihole-kubernetes/pull/274 - So I'm closing your pull request. Thank you nonetheless

thegreatsunra commented 5 months ago

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