Remove frontend container for new merged docker #29
konrad
Loading…
x
Reference in New Issue
Block a user
No description provided.
Delete Branch "xeruf/helm-chart:merged-docker"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Currently testing on my cluster
Fixes #28
It works! Doing further validation now...
Oddly importing our data from 0.22.1 succeeds but if I look inside VIkunja I cannot find it
Does it exist in the database?
@ -140,3 +145,3 @@Anything you see [in bjw-s' `common` library](https://github.com/bjw-s/helm-charts/blob/a081de53024d8328d1ae9ff7e4f6bc500b0f3a29/charts/library/common/values.yaml),including the top-level keys, can be added and subtracted from this chart's `values.yaml`,underneath the `api`, `frontend`, and (optionally) `typesense` key.underneath the `api` and (optionally) `typesense` key.What do you think about renaming the
apikey? Since it's no longer only the api.planning to rename it to
vikunja, wanted to verify everything runs fine firstThis is the output from my helper-script:
The problem is I have no decent way of inspecting the pod because somehow the new docker container does not even have a shell or ls or anything?
The import definitely did not work, we have multiple teams:
Nothing after that? Are you sure it crashed and not that it just continued importing without printing anything?
Yes, it only contains the Vikunja binary. That reduces the attack surface and makes it more maintainable because we don't need to "maintain" the OS in the image. There is nothing else in that container, so what do you need the shell for anyways?
The scripts terminates at that point, not even an indication of it having crashed:
I want to check that the backup file has been uploaded correctly.
Does
kubectlforward the exit code from the command it ran?Can't you check that in the volume?
The file is not even read
Can you update this to 0.24.1?
Working on it finally again ;)
Update pushed, don't know how to test it locally so I would suggest we publish and then I test it
WIP: Remove frontend container for new merged dockerto Remove frontend container for new merged dockerThe last commit is still 7 months old, can you push again?
After pulling latest unstable it says
Vikunja version v0.24.1-566-b3040b8466even though unstable is 0.24.5, huh? https://hub.docker.com/r/vikunja/vikunja/tagsAnd then I get
2024-12-11T21:42:51Z: CRITICAL ▶ 0f2 It looks like your openid configuration is in the wrong format. Please check the docs for the correct format.Format is the same as before, did anything change?
https://kolaente.dev/xeruf/helm-chart/src/branch/merged-docker
I pushed, but something interesting is going on:
and I cannot upload the backup to test the restore properly either ^^
I've re-run the action in the admin dashboard, can you push again?
What does the command do? The Vikunja container only contains the Vikunja binary and nothing else.
managed to copy to the folder and initiate restore, but now I get:
2024-12-12T11:02:51Z: CRITICAL ▶ 070 export was created with version 0.20.4 but this is 0.24.1-566-b3040b8466 - please make sure you are running the same Vikunja version before restoring
How is that supposed to work?
still get the message on the repo:
Git hooks of this repository seem to be broken. Please follow the documentation to fix them, then push some commits to refresh the status.As the message says, you can't import a dump created on an older version of Vikunja.
Alright, downgraded to 0.20.4, restored and then upgraded - and ended up with
2024-12-12T16:40:38Z: CRITICAL ▶ 065 Migration failed: migration 20221228112131 failed: pq: duplicate key value violates unique constraint "lists_pkey"againwhat about the openid config?
dumping and importing our latest data with version 0.22.1 also produces an error: vikunja/vikunja#2934
nevermind that one, remembered how to test it now ^^
4abc90f129to95fb307ff395fb307ff3to1a1496bcafRemove frontend container for new merged dockerto WIP: Remove frontend container for new merged docker1a1496bcafto8e130ffc68phew, seems I finally have a working version
openid works now, the only change I did was to add the
scopefield but removing it again also does not seem to make a difference ^^my fork is also in order again, thanks :)
The config format has been changed recently, but that's not yet released. Are you using a stable release or an unstable build?
Is this ready to review now? (still marked WIP)
@ -38,4 +36,1 @@url: https://vikunja.io- name: Yurii Vlasovemail: yuriy@vlasov.prourl: https://vlasov.proWhy did you remove Yurii?
He does not seem active anymore, I understand this not as credits but as active maintainers so instead of adding in me and the others I thought keeping it minimal is best. Other charts I saw kept it similarly
That makes sense.
@ -50,4 +50,1 @@# The configuration for Vikunja's api.# https://vikunja.io/docs/config-options/VIKUNJA_SERVICE_FRONTENDURL: "http://{{ index .Values.frontend.ingress.main.hosts 0 "host" }}{{ index .Values.frontend.ingress.main.hosts 0 "path" }}"{{ end }}Isn't this
{{ end }}too much now?nope it ends the hardcoded values definition
Ahhh, I was looking for an opening
{{ if }}@ -85,4 +62,0 @@type: ClusterIP# https://github.com/bjw-s/helm-charts/blob/a081de53024d8328d1ae9ff7e4f6bc500b0f3a29/charts/library/common/values.yaml#L393-L436ingress:Don't we still need an ingress?
still present, this is just the frontend
gotcha.
@ -18,2 +13,2 @@tag: 0.21.0pullPolicy: IfNotPresentrepository: vikunja/vikunja#tag: "stable"This tag does not exist.
my bad, correction coming up
ah, I was intermittently using unstable, that must have been it
sort of - I am inclined to merge it as is and then make more major updates such as when updating the bjw subchart as I might need some helpe there and there is no urgency to it: https://github.com/bjw-s/helm-charts/issues/301#issuecomment-2541109674
@ -12,3 +10,2 @@category: TaskTrackerversion: 0.4.3appVersion: 0.21.0version: 1.0.0Why 1.0.0 instead of 0.5.0?
Because this update is a breaking change, and I don't find dragging out a 1.0 productive anyway, also for Vikunja, it just makes the first number meaningless (See Java, where they dropped it at some point).
For exxample for Vikunja, I believe the upgrade 0.22 to 0.23 should have been a major bump since it changes the structure majorly.
For me 0.X is for software that is not properly usable, which Vikunja and this helm chart are long past.
I see your point, but you also said you want to merge this as is and then fix it later. That's not a definition of the very first 1.0 release 🙂
@ -11,4 +8,4 @@# VIKUNJA COMPONENTS ######################## You can find the default values that this `values.yaml` overrides, in the comment at the top of this file.api:This might need renaming as it is not only the api anymore?
was deliberating to put that into v2.0 but maybe now is better yeah
I'm fine to merge this as long as it works.
What's the status here?
WIP: Remove frontend container for new merged dockerto Remove frontend container for new merged dockerI am now testing this on two instances, one old one new, but am yet to bring our old production instance over.
Polished and documented things so I think this is pretty much ready.
@ -5,3 +5,3 @@- name: postgresqlrepository: https://charts.bitnami.com/bitnamiversion: 12.1.9version: 16.3.0Is this postgres version 16? Because if we're upgrading this, why not upgrade directly to postgres 17?
It is postgres 17, this is the chart version ;)
Gotcha!
Looks like 16.4.5 is out btw
@ -19,1 +13,3 @@pullPolicy: IfNotPresentrepository: vikunja/vikunja#tag: "latest"#pullPolicy: AlwaysWhy comment this?
Cause a specific chart version should always deploy a specific app version by default, which comes from the
Chart.yaml. Commented sections are as reference for users.If it's not used, it should be removed. The reference and possible options should be documented elsewhere.
Also right now it's not specified what it pulls, IMHO this should include the tag (but maybe not latest)
it is specified in
Chart.yaml, I would not want to duplicate itthere are other options which are also commented out, having the values.yaml as a reference is common practice in helm charts
Just to add a datapoint, I was able to get this working in my environment with very little effort, nice work! I did find one minor annoyance in that the Typesense URL needs to be prefixed with
http://, e.g.Otherwise it worked well with no modification, just some configuration of the values.
@ -0,0 +41,4 @@{{ if .Values.redis.enabled }}VIKUNJA_REDIS_ENABLED: "true"{{ end }}{{ if .Values.typesense.enabled }}Would be nice to add the following to this file to prevent permissions problems with uploads:
the right location to add it is probably templates/typesense.yaml - feel free to contribute an appropriate adjustment as I am not using typesense so I do not want to mess with it
Did you perchance mean for this to be on my other comment? This one is needed for the Vikunja uploads (e.g. backgrounds, attachments, etc) because of the container user/group ID.
ah you commented on the typesense values so I thought that was what it was about - if you know where to add it please comment appropriately and I wouldn't interfere ;) but what is the issue without that tag exactly?
Ah, yeah there's an open bug with Gitea that makes it impossible to comment on a non-diffed line. As for why this is necessary, none of the attachments/file upload functionality will work without this because the persistent volumes that get created will not be owned by the correct group. It can just go anywhere in this file.
Just passing by and I'm wondering about
If someone doesn't use nginx but something else like traefik this line is pretty much useless
feel free to complement it for other tools, but there is no harm in including it, right?
I consider this ready to merge.
The new comments on here could be own contributions or issues, they are not really relevant to this PR in particular.
Well, the
fsGroupissue means that file upload functionality is broken, so I'd recommend fixing that at least.@sailorbob134280 we did not have any issues with uploading on our instance but I added it in anyways
Is there any chance you were reusing existing PVs (e.g. from a previous deployment)? That's odd to me it would work because AFAIK it shouldn't. But since the container runs with group 1000, this shouldn't hurt anything.
ah I use predefined PVs anyways, not the ones from the chart, for backup purposes