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

existingSecret for users and configurable accessMode for Samba #130

Merged
merged 3 commits into from
Jul 20, 2023

Conversation

mosanden
Copy link
Contributor

@mosanden mosanden commented Jul 18, 2023

What this PR does / why we need it:

  • Enable usage of existingSecret for Samba users: It is now possible to define users separate from the chart, e.g. in a SealedSecret.

  • Make accessMode for volumes configurable in Samba: For example hostPath PVs do not support ReadWriteMany.

Checklist

  • Chart Version bumped
  • make docs lint passes
  • Variables are documented in the values.yaml as required in Helm-Docs.
  • PR contains the label that identifies the chart, e.g. chart/[chart]
  • PR contains the label that identifies the type of change, which is one of
    bug, enhancement, documentation, change, breaking, dependency

Signed-off-by: Moritz Sanden <mo.sanden@mail.de>
@mosanden
Copy link
Contributor Author

Hey there, this PR needs the labels change and chart/samba i guess but I don't understand how I can add them.

@mosanden mosanden marked this pull request as draft July 18, 2023 20:38
@ccremer ccremer added enhancement New feature or request chart/samba labels Jul 18, 2023
@ccremer
Copy link
Owner

ccremer commented Jul 18, 2023

Hi!
Thanks for the interest and PR!
The change overall looks good to me :)

In CI the linting is failing, looks like you forgot a small change. However, the unit test job might need some investigation. It's been a while since I worked on the charts, so we might skip that one, unless you find the error immediately :)

@mosanden
Copy link
Contributor Author

mosanden commented Jul 18, 2023

Seems like I didn't generate the docs correctly.

For the tests I get this for a local make test:

$ make test
--- Executing unit tests
ok      github.com/ccremer/charts/charts/emby/test      (cached)
ok      github.com/ccremer/charts/charts/fronius-exporter/test  (cached)
ok      github.com/ccremer/charts/charts/znapzend/test  (cached)

@mosanden mosanden marked this pull request as ready for review July 19, 2023 08:21
Copy link
Owner

@ccremer ccremer left a comment

Choose a reason for hiding this comment

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

I propose a slight change in the parameter, allowing more flexibility. Otherwise LGTM

charts/samba/templates/deployment.yaml Outdated Show resolved Hide resolved
charts/samba/templates/secret.yaml Outdated Show resolved Hide resolved
charts/samba/values.yaml Outdated Show resolved Hide resolved
charts/samba/README.md Outdated Show resolved Hide resolved
@mosanden mosanden marked this pull request as draft July 19, 2023 14:47
@mosanden
Copy link
Contributor Author

I like that idea. I'll check if it works.

Signed-off-by: Moritz Sanden <mo.sanden@mail.de>
Signed-off-by: Moritz Sanden <mo.sanden@mail.de>
@mosanden mosanden marked this pull request as ready for review July 19, 2023 21:08
@mosanden
Copy link
Contributor Author

Seems to work!

@ccremer ccremer merged commit 7e40f3c into ccremer:master Jul 20, 2023
4 checks passed
@ccremer
Copy link
Owner

ccremer commented Jul 20, 2023

Perfect!
Thank you for your contribution and time :)

@mosanden
Copy link
Contributor Author

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chart/samba enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants