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

fix(paperless-ng): fix redis URL and use default paths #1560

Merged
merged 18 commits into from Dec 20, 2021

Conversation

stavros-k
Copy link
Member

Description

Fixes #

Type of change

  • Feature/App addition
  • Bugfix
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Refactor of current code

How Has This Been Tested?

Notes:

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests to this description that prove my fix is effective or that my feature works
  • I increased versions for any altered app according to semantic versioning

@stavros-k
Copy link
Member Author

Added redisDatabase: "0" in values.yaml to render url correctly, without it the url renders as follows:

PAPERLESS_REDIS=redis://paperless-ng:tS41isrcRUiMZEpg7z3BueNfqpoJLPzdhHcpXQvwPkrmXSmaiy@tset-redis:5432/<nil>

@stavros-k stavros-k changed the title fix(paperless-ng): add log storage persistence fix(paperless-ng): fix redis URL and use default paths Dec 18, 2021
@stavros-k
Copy link
Member Author

It should be ok now, just need to manually test once PR #1561 for common is merged.

@Ornias1993
Copy link
Member

Note: You should add this to redis instead afaik.

@stavros-k
Copy link
Member Author

Note: You should add this to redis instead afaik.

Hopefully I did that right :)
#1563

@Ornias1993
Copy link
Member

Lets keep the paths clear and root-centered instead of hidden behind /usr/something

@stavros-k
Copy link
Member Author

Lets keep the paths clear and root-centered instead of hidden behind /usr/something

That was my first intention, but I got permission denied on /config/log dir which was under pvc.
The app is running as root without rofs..

@Ornias1993
Copy link
Member

Ornias1993 commented Dec 18, 2021

Lets keep the paths clear and root-centered instead of hidden behind /usr/something

That was my first intention, but I got permission denied on /config/log dir which was under pvc. The app is running as root without rofs..

permissions for PVC's are handled by kubernetes, regardless of the mountPath.

@stavros-k
Copy link
Member Author

Lets keep the paths clear and root-centered instead of hidden behind /usr/something

That was my first intention, but I got permission denied on /config/log dir which was under pvc. The app is running as root without rofs..

permissions for PVC's are handled by kubernetes, regardless of the mountPath.

Once the new common is released, i'll try manually something else to move to shorter paths.
Seems that path vars need to be set relative, not absolute...

@stavros-k
Copy link
Member Author

This should be merged last (after the cleanup PR's), after it's rebased ofc.

@stavros-k stavros-k marked this pull request as ready for review December 19, 2021 22:45
Copy link
Member

@Ornias1993 Ornias1993 left a comment

Choose a reason for hiding this comment

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

Please fix the weird paths.

@stavros-k
Copy link
Member Author

Please fix the weird paths.

Whoops, right! I'll do tomorrow though as I have to test how the heck does the app interpretes the paths. It's probably relative and not absolute..!

@Ornias1993
Copy link
Member

In that case i'm going to push this back to draft, do the last patch bump and branch off for RC2

@Ornias1993 Ornias1993 marked this pull request as draft December 19, 2021 23:00
@stavros-k
Copy link
Member Author

Lets keep the paths clear and root-centered instead of hidden behind /usr/something

That was my first intention, but I got permission denied on /config/log dir which was under pvc. The app is running as root without rofs..

permissions for PVC's are handled by kubernetes, regardless of the mountPath.

Okay, I think I found something..

This is on values.yaml

  USERMAP_UID: "{{ .Values.env.PUID }}"
  USERMAP_GID: "{{ .Values.env.PGID }}"

This is section of the output of k describe pod -n ix-test test-paperless-ng-7ccd5c9d7b-j2sll

    Environment:
      PGID:                       568
      GROUP_ID:                   568
      GID:                        568
      PAPERLESS_DBHOST:           <set to the key 'plainhost' in secret 'dbcreds'>                         Optional: false
      PAPERLESS_DBPASS:           <set to the key 'postgresql-password' in secret 'dbcreds'>               Optional: false
      PAPERLESS_REDIS:            <set to the key 'url' in secret 'rediscreds'>                            Optional: false
      PAPERLESS_SECRET_KEY:       <set to the key 'PAPERLESS_SECRET_KEY' in secret 'paperlessng-secrets'>  Optional: false
      PAPERLESS_CONSUMPTION_DIR:  /consume
      PAPERLESS_DATA_DIR:         /config
      PAPERLESS_DBNAME:           paperless-ng
      PAPERLESS_DBPORT:           5432
      PAPERLESS_DBUSER:           paperless-ng
      PAPERLESS_LOGGING_DIR:      /logs
      PAPERLESS_MEDIA_ROOT:       /media
      PAPERLESS_TIME_ZONE:        Europe/Athens
      PUID:                       568
      TZ:                         Europe/Athens
      UMASK:                      002
      USERMAP_GID:
      USERMAP_UID:                568

While PGID, GID and GROUP_ID is correctly set to 568 from fsGroup...
As you can see USERMAP_GID is not populated. And defaulting to group 1000, which doesn't have the write perms.
(fsGroup is set to 568 on installation wizard).

One solution would be to change it to USERMAP_GID: "{{ .Values.podSecurityContext.fsGroup }}. Because USERMAP_GID seems to render before PGID.

Or I'm just not seeing something really obvious... I'll do more tests tomorrow..

@Ornias1993
Copy link
Member

yes, set it to fsgroup

Comment on lines 208 to 210
- variable: config
label: "App Config Storage"
label: "App Data Storage"
description: "This is where paperless stores all its data (search index, classification model, etc)"
Copy link
Member

Choose a reason for hiding this comment

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

if you want to call it data, use /data instead of /config

@Ornias1993
Copy link
Member

This should fix up most of the dirt.

@Ornias1993 Ornias1993 marked this pull request as ready for review December 20, 2021 16:42
@Ornias1993 Ornias1993 merged commit 0710b35 into truecharts:master Dec 20, 2021
@stavros-k stavros-k deleted the fixPaperless branch December 20, 2021 16:42
@truecharts-admin
Copy link
Collaborator

This PR is locked to prevent necro-posting on closed PRs. Please create a issue or contact staff on discord if you want to further discuss this

@truecharts truecharts locked as resolved and limited conversation to collaborators Jun 2, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants