Fix regression for creating repositories in root-based containers #172

Merged
luhahn merged 2 commits from justusbunsi/helm-chart:issue/171-repo-api into master 2021-06-09 14:35:51 +00:00
Member

Due to #160 it was no longer possible to create repositories in root-based containers. This was caused by the missing /tmp/gitea directory in that image. It was dynamically created by Gitea internal functionality with less privileges than necessary.

Explicitly creating the directory and set proper permissions fix this.

Fixes: #171

Due to #160 it was no longer possible to create repositories in root-based containers. This was caused by the missing `/tmp/gitea` directory in that image. It was dynamically created by Gitea internal functionality with less privileges than necessary. Explicitly creating the directory and set proper permissions fix this. Fixes: #171
Author
Member

@luhahn Does the increase of Chart.yaml app version does the trick to indicate the correct version for the chart? As mentioned in issue #171, this was incorrect due to the previous PR #160.

@luhahn Does the increase of Chart.yaml app version does the trick to indicate the correct version for the chart? As mentioned in issue #171, this was incorrect due to the previous PR #160.
Member

@luhahn Does the increase of Chart.yaml app version does the trick to indicate the correct version for the chart? As mentioned in issue #171, this was incorrect due to the previous PR #160.

Yes, missed it in your PR, sorry :D.

> @luhahn Does the increase of Chart.yaml app version does the trick to indicate the correct version for the chart? As mentioned in issue #171, this was incorrect due to the previous PR #160. Yes, missed it in your PR, sorry :D.
luhahn requested changes 2021-06-09 07:34:52 +00:00
@ -24,0 +25,4 @@
mkdir -p "${GITEA_TEMP}"
chown 1000:1000 "${GITEA_TEMP}"
chmod ug+rwx "${GITEA_TEMP}"
Member

Since we need this only on the root image, should we rebase your branch on my
Pr #165 so we can include your checks into the rootless if condition?

Something like this:

{{- if not .Values.image.rootless }}
    chown 1000:1000 /data
    mkdir -p "${GITEA_TEMP}"
    chown 1000:1000 "${GITEA_TEMP}"
    chmod ug+rwx "${GITEA_TEMP}"
{{- end }}
Since we need this only on the root image, should we rebase your branch on my Pr https://gitea.com/gitea/helm-chart/pulls/165 so we can include your checks into the rootless if condition? Something like this: ```yaml {{- if not .Values.image.rootless }} chown 1000:1000 /data mkdir -p "${GITEA_TEMP}" chown 1000:1000 "${GITEA_TEMP}" chmod ug+rwx "${GITEA_TEMP}" {{- end }} ```
Author
Member

It wouldn't cause any issues when running for both image variants. In the root image this is necessary. For the rootless image this would fix permissions in case they are too restrictive. At least that's what I've tested so far.

Actually, I don't think that the chown 1000:1000 /data is necessary since the fsGroup already declares the group as 1000. It should work without the chown-in.

I leave it up to you to decide whether I should rebase or not. ?

Will update the readme.

It wouldn't cause any issues when running for both image variants. In the root image this is necessary. For the rootless image this would fix permissions in case they are too restrictive. At least that's what I've tested so far. Actually, I don't think that the `chown 1000:1000 /data` is necessary since the `fsGroup` already declares the group as 1000. It _should_ work without the chown-in. I leave it up to you to decide whether I should rebase or not. ? Will update the readme.
Member

Yea, I agree, it SHOULD :D. But it doesn't we had an issue (#135) and i could only resolve it by applying the chown.

However, i just tested your changes and yes, the chown doesn't cause any error messages :)

You'll still need to update your branch with the latest master, so we can merge

Yea, I agree, it SHOULD :D. But it doesn't we had an issue (https://gitea.com/gitea/helm-chart/issues/135) and i could only resolve it by applying the chown. However, i just tested your changes and yes, the chown doesn't cause any error messages :) You'll still need to update your branch with the latest master, so we can merge
6543 marked this conversation as resolved
Member

Please also add the image version into the readmes value description

Please also add the image version into the readmes value description
Author
Member

Please also add the image version into the readmes value description

Seems to be already done in 031b58c90e.

> Please also add the image version into the readmes value description Seems to be already done in 031b58c90ea3b18eb8964fab5b1f214e78fca79b.
Member

Please also add the image version into the readmes value description

Seems to be already done in 031b58c90e.

right, missed that one, sorry

> > Please also add the image version into the readmes value description > > Seems to be already done in 031b58c90ea3b18eb8964fab5b1f214e78fca79b. right, missed that one, sorry
luhahn approved these changes 2021-06-09 11:53:06 +00:00
justusbunsi force-pushed issue/171-repo-api from 109b89ec5f to 9a7f67b406 2021-06-09 12:14:56 +00:00 Compare
Author
Member

Branch up-to-date. Hopefully these changes do not introduce other side-effects. ?

Branch up-to-date. Hopefully these changes do not introduce other side-effects. ?
6543 approved these changes 2021-06-09 13:39:02 +00:00
luhahn merged commit 6e841e6e26 into master 2021-06-09 14:35:51 +00:00
Sign in to join this conversation.
No description provided.