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

feat: support subPath for library #118

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

Conversation

ilazycat
Copy link

@ilazycat ilazycat commented Sep 2, 2024

It will be handy if we can set subPath in library config so we don't have to create and manage separate pv for library/machine-learning-models/postgre/redis.

It allows us to do something like this:

image:
  tag: v1.113.0

immich:
  metrics:
    enabled: false
  persistence:
    library:
      existingClaim: immich
      subPath: data

machine-learning:
  persistence:
    cache:
      enabled: true
      type: null # Have to set this to null otherwise emptyDir will be used.
      path: /cache
      existingClaim: immich
      subPath: ml-model-cache

postgresql:
  enabled: true
  primary:
    persistence:
      existingClaim: immich
      subPath: postgresql

redis:
  enabled: true
  auth:
    enabled: false
  master:
    persistence:
      existingClaim: immich
      subPath: redis

@bo0tzz
Copy link
Member

bo0tzz commented Sep 2, 2024

Thanks for the PR! Putting multiple not-quite-related things into the same PVC feels like an antipattern to me, so I'd rather not add a specific option for that. However, if you want to contribute a change that makes it possible to set any of the upstream values in the library persistence block in a generic way, that would be welcome.

@ilazycat
Copy link
Author

ilazycat commented Sep 2, 2024

@bo0tzz I think you are right. I updated the code, using toYaml function to make it possible to set any of the values under persistence.library.

@ilazycat
Copy link
Author

ilazycat commented Sep 2, 2024

After some quick testing I found it possible to use hostPath as library.

  persistence:
    library:
      existingClaim: immich
      type: hostPath
      hostPath: /data/
      subPath: immich

However existingClaim is still required since there's some checking done in templates/checks.yaml

@bo0tzz
Copy link
Member

bo0tzz commented Sep 3, 2024

Intuitively the toYaml approach doesn't feel correct to me, but I'm not familiar enough with helm to suggest anything better 🤔

@ilazycat
Copy link
Author

ilazycat commented Sep 3, 2024

Actually I've tried several other methods. They are either not working or didn't get along well with bjw-s' common chart library, not giving the desired output. toYaml is the best working method in my test. Yet I'm not a helm expert as well so I'll let you decide.

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.

2 participants