gitea/helm-gitea
Fork 169
Code Issues 54 Pull Requests 11 Actions 2 Releases 103 Activity

Add image.fullOverride #550

Merged
pat-s merged 11 commits from TristanHoladay/helm-chart:feature-decouple-rootless into main 2023-11-14 21:42:27 +00:00
Conversation 8 Commits 11 Files Changed 6 +100 -14
TristanHoladay commented 2023-11-02 15:31:28 +00:00
Contributor
Copy Link

Description of the change

This PR is a continuation of the work done by @dgershman in 534, to allow users to override the image from the default rootless behavior of appending -rootless to the end of the image tag.

Benefits

Allows more flexibility to use externally maintained images that are rootless but don't follow the -rootless tag convention.

Applicable issues

Additional information

No breaking changes. This does not affect the image.rootless conditional checks or the current behavior if someone still wants to rely on the chart to append -rootless.

Checklist

  • Parameters are documented in the values.yaml and added to the README.md using readme-generator-for-helm
  • Breaking changes are documented in the README.md
  • Templating unittests are added
### Description of the change This PR is a continuation of the work done by @dgershman in [534](https://gitea.com/gitea/helm-chart/pulls/534), to allow users to override the image from the default rootless behavior of appending `-rootless` to the end of the image tag. ### Benefits Allows more flexibility to use externally maintained images that are rootless but don't follow the `-rootless` tag convention. ### Applicable issues - fixes #532 ### Additional information No breaking changes. This does not affect the `image.rootless` conditional checks or the current behavior if someone still wants to rely on the chart to append `-rootless`. ### Checklist - [x] Parameters are documented in the `values.yaml` and added to the `README.md` using [readme-generator-for-helm](https://github.com/bitnami-labs/readme-generator-for-helm) - [x] Breaking changes are documented in the `README.md` - [x] Templating unittests are added
👍 1
TristanHoladay added 2 commits 2023-11-02 15:31:28 +00:00
feat: decouple image from rootless value 0f0e4316ee
wip: updated README and refactored init_directory_structure.sh-rootless test
All checks were successful
check-and-test / check-and-test (pull_request) Successful in 31s
Details
cafa85315e
TristanHoladay added 1 commit 2023-11-02 16:11:59 +00:00
Merge branch 'main' into feature-decouple-rootless
All checks were successful
check-and-test / check-and-test (pull_request) Successful in 34s
Details
1ac58b857f
pat-s added 1 commit 2023-11-06 08:17:42 +00:00
Merge branch 'main' into feature-decouple-rootless
All checks were successful
check-and-test / check-and-test (pull_request) Successful in 27s
Details
6285dba31e
TristanHoladay added 1 commit 2023-11-08 13:59:38 +00:00
Merge branch 'main' into feature-decouple-rootless
All checks were successful
check-and-test / check-and-test (pull_request) Successful in 39s
Details
f3c13843ea
pat-s commented 2023-11-09 15:37:27 +00:00
Collaborator
Copy Link

Thanks! Also for adding tests. I think one important thing to add would be an entry to README which lists the adaptions users need to make when providing their own rootless image and running rootless: false. Or, phrased differently, we should make it explicit what using rootless: true does behind the scences. They could be added in a new subsection below https://gitea.com/gitea/helm-chart#user-content-configuration. AFAICS these are:

  • Changing $HOME to /data/gitea/git
  • Setting START_SSH_SERVER: true
  • Setting SSH_LISTEN_PORT: 2222
  • (optional) Defining SSH_LOG_LEVEL

Other actions, like chowning /data are only important when running rootful.

@justusbunsi Feel free to add tasks in case I forgot some actions.

Thanks! Also for adding tests. I think one important thing to add would be an entry to README which lists the adaptions users need to make when providing their own rootless image and running `rootless: false`. Or, phrased differently, we should make it explicit what using `rootless: true` does behind the scences. They could be added in a new subsection below https://gitea.com/gitea/helm-chart#user-content-configuration. AFAICS these are: - Changing `$HOME` to `/data/gitea/git` - Setting `START_SSH_SERVER: true` - Setting `SSH_LISTEN_PORT: 2222` - (optional) Defining `SSH_LOG_LEVEL` Other actions, like chowning `/data` are only important when running rootful. @justusbunsi Feel free to add tasks in case I forgot some actions.
TristanHoladay added 1 commit 2023-11-09 19:41:58 +00:00
wip: updating README with info on what image.rootless:true does
Some checks failed
check-and-test / check-and-test (pull_request) Failing after 30s
Details
c9d701c2f3
TristanHoladay commented 2023-11-09 19:43:56 +00:00
Author
Contributor
Copy Link

@pat-s thank you for the feedback. I just pushed and update based on your suggestion. Not sure if I really hit the mark with what you're wanting but just let me know what you think should be altered and if i got the location wrong and I'm happy to make those changes.

@pat-s thank you for the feedback. I just pushed and update based on your suggestion. Not sure if I really hit the mark with what you're wanting but just let me know what you think should be altered and if i got the location wrong and I'm happy to make those changes.
justusbunsi requested changes 2023-11-11 18:03:04 +00:00
justusbunsi left a comment
Contributor
Copy Link

Thanks for continuing the work of #534.

Thanks for continuing the work of #534.
README.md Outdated
@@ -172,6 +172,26 @@ The Prometheus `/metrics` endpoint is disabled by default.
ENABLED = false
```
#### Rootless Defaults
justusbunsi commented 2023-11-11 17:00:45 +00:00
Contributor
Copy Link

Please update the table-of-content at the top of this document.

Please update the table-of-content at the top of this document.
👍 1
justusbunsi marked this conversation as resolved
README.md Outdated
@@ -174,1 +174,4 @@
#### Rootless Defaults
If `.Values.image.rootless: true`, then the following will occur:
justusbunsi commented 2023-11-11 17:40:44 +00:00
Contributor
Copy Link

Suggestion:

- If `.Values.image.rootless: true`, then the following will occur: + If `.Values.image.rootless: true`, then the following will occur. In case you use `.Values.image.fullOverride`, check that this works in your image:

Referring to the new option makes it more obvious why this is an important information.

Suggestion: ```diff - If `.Values.image.rootless: true`, then the following will occur: + If `.Values.image.rootless: true`, then the following will occur. In case you use `.Values.image.fullOverride`, check that this works in your image: ``` Referring to the new option makes it more obvious why this is an important information.
👍 1
justusbunsi marked this conversation as resolved
README.md Outdated
@@ -175,0 +178,4 @@
- `$HOME` becomes `/data/gitea/git`
[see deployment.yaml](./templates/gitea/deployment.yaml#L185)
justusbunsi commented 2023-11-11 17:55:30 +00:00
Contributor
Copy Link

Be careful with referencing specific lines of code. They tend to get outdated or incomplete quite fast. Instead, let the user look for contexts: see [deployment.yaml](./templates/gitea/deployment.yaml) template inside (init-)container "env" declarations.

Be careful with referencing specific lines of code. They tend to get outdated or incomplete quite fast. Instead, let the user look for contexts: `see [deployment.yaml](./templates/gitea/deployment.yaml) template inside (init-)container "env" declarations`.
👍 1
justusbunsi marked this conversation as resolved
README.md Outdated
@@ -175,0 +188,4 @@
[see \_helpers.tpl](./templates/_helpers.tpl#L340)
- (optional) Defining `SSH_LOG_LEVEL`
justusbunsi commented 2023-11-11 17:58:42 +00:00
Contributor
Copy Link

Changing SSH_LOG_LEVEL is only possible via Chart logic when not using rootless images. See 7de8e83433/templates/gitea/deployment.yaml (L270) for the condition.

Changing `SSH_LOG_LEVEL` is only possible via Chart logic when _not_ using `rootless` images. See https://gitea.com/gitea/helm-chart/src/commit/7de8e834330c1a9cb1de3aae70c2076970f79875/templates/gitea/deployment.yaml#L270 for the condition.
👍 1
justusbunsi marked this conversation as resolved
README.md Outdated
@@ -1084,12 +1105,14 @@ gitea:
```
<!-- markdownlint-disable-next-line -->
justusbunsi commented 2023-11-11 17:03:22 +00:00
Contributor
Copy Link

The empty lines between <!-- markdownlint-disable-next-line --> (4x) causes the build to fail. Please remove these empty lines. They are probably added automatically.

The empty lines between `<!-- markdownlint-disable-next-line -->` (4x) causes the build to fail. Please remove these empty lines. They are probably added automatically.
👍 1
justusbunsi marked this conversation as resolved
unittests/deployment/image-configuration.yaml Outdated
@@ -60,0 +67,4 @@
asserts:
- equal:
path: spec.template.spec.containers[0].image
value: "gitea/gitea:1.19.3"
justusbunsi commented 2023-11-11 18:00:45 +00:00
Contributor
Copy Link

Kudos for demonstrating how and ensuring that the image.digest field is completely ignored when using image.fullOverride.

Kudos for demonstrating how and ensuring that the `image.digest` field is completely ignored when using `image.fullOverride`.
TristanHoladay commented 2023-11-13 14:10:37 +00:00
Author
Contributor
Copy Link

Thanks! I was thinking of even adding the registry, repository, and tag. Do you think that's worth doing, or is digest enough?

Thanks! I was thinking of even adding the registry, repository, and tag. Do you think that's worth doing, or is digest enough?
justusbunsi commented 2023-11-13 17:21:32 +00:00
Contributor
Copy Link

That would be great. You can even add a code comment that this purposely set to ensure such behavior.

That would be great. You can even add a code comment that this purposely set to ensure such behavior.
👍 1
justusbunsi marked this conversation as resolved
values.yaml Outdated
@@ -43,6 +43,7 @@ clusterDomain: cluster.local
## @param image.digest Image digest. Allows to pin the given image tag. Useful for having control over mutable tags like `latest`
## @param image.pullPolicy Image pull policy
## @param image.rootless Wether or not to pull the rootless version of Gitea, only works on Gitea 1.14.x or higher
## @param image.fullOverride Completely overrides the image registry, path/image, tag and digest
justusbunsi commented 2023-11-11 17:22:49 +00:00
Contributor
Copy Link

Suggestion:

- ## @param image.fullOverride Completely overrides the image registry, path/image, tag and digest + ## @param image.fullOverride Completely overrides the image registry, path/image, tag and digest. **Adjust `image.rootless` accordingly and review [Rootless defaults](#rootless-defaults).**

This makes it more obvious that the Chart logic still rely on that setting, even when image.fullOverride is used. And gives a hint to the new section.

Suggestion: ```diff - ## @param image.fullOverride Completely overrides the image registry, path/image, tag and digest + ## @param image.fullOverride Completely overrides the image registry, path/image, tag and digest. **Adjust `image.rootless` accordingly and review [Rootless defaults](#rootless-defaults).** ``` This makes it more obvious that the Chart logic still rely on that setting, even when `image.fullOverride` is used. And gives a hint to the new section.
👍 1
justusbunsi marked this conversation as resolved
justusbunsi changed title from feature-decouple-rootless to Add `image.fullOverride` 2023-11-11 19:13:06 +00:00
TristanHoladay added 1 commit 2023-11-13 13:41:19 +00:00
wip: README rootles defaults refactors
All checks were successful
check-and-test / check-and-test (pull_request) Successful in 32s
Details
42439e6ef2
TristanHoladay commented 2023-11-13 14:18:49 +00:00
Author
Contributor
Copy Link

Thank you for the awesome feedback and suggestions @justusbunsi.

Thank you for the awesome feedback and suggestions @justusbunsi.
justusbunsi reviewed 2023-11-13 17:34:06 +00:00
README.md Outdated
@@ -175,0 +189,4 @@
[see \_helpers.tpl](./templates/_helpers.tpl) in `gitea.inline_configuration.defaults.server` definition
- Defining `SSH_LOG_LEVEL` turned off
justusbunsi commented 2023-11-13 17:33:44 +00:00
Contributor
Copy Link

If I got your intention right, are trying to say that the SSH_LOG_LEVEL environment variable is not injected into the container when using a rootless image. If that's the case, I suggest writing it that way. The current "Defining SSH_LOG_LEVEL turned off" sounds a bit off.

If I got your intention right, are trying to say that the `SSH_LOG_LEVEL` environment variable is not injected into the container when using a rootless image. If that's the case, I suggest writing it that way. The current "Defining `SSH_LOG_LEVEL` turned off" sounds a bit off.
👍 1
justusbunsi marked this conversation as resolved
TristanHoladay added 1 commit 2023-11-13 17:37:28 +00:00
wip: update image fullOverride unit test
All checks were successful
check-and-test / check-and-test (pull_request) Successful in 30s
Details
64d21effed
TristanHoladay added 1 commit 2023-11-13 17:37:53 +00:00
Merge branch 'main' into feature-decouple-rootless
All checks were successful
check-and-test / check-and-test (pull_request) Successful in 29s
Details
d376fc26f5
TristanHoladay added 1 commit 2023-11-13 17:44:35 +00:00
wip: update README rootless defaults log level info
All checks were successful
check-and-test / check-and-test (pull_request) Successful in 28s
Details
5de56902f1
TristanHoladay added 1 commit 2023-11-14 15:43:33 +00:00
Merge branch 'main' into feature-decouple-rootless
All checks were successful
check-and-test / check-and-test (pull_request) Successful in 33s
Details
236beb1a99
justusbunsi approved these changes 2023-11-14 16:16:03 +00:00
justusbunsi left a comment
Contributor
Copy Link

Awesome 🚀

Awesome 🚀
justusbunsi commented 2023-11-14 16:59:50 +00:00
Contributor
Copy Link

@pat-s Anything left from your side?

@pat-s Anything left from your side?
pat-s approved these changes 2023-11-14 21:42:19 +00:00
pat-s left a comment
Collaborator
Copy Link

I'll add some post-merge wording and formatting updates but overall 👍 Thanks everyone!

I'll add some post-merge wording and formatting updates but overall 👍 Thanks everyone!
pat-s merged commit 3cf91bf6e7 into main 2023-11-14 21:42:27 +00:00
Sign in to join this conversation.
Reviewers
No Reviewers
justusbunsi
pat-s
Labels
Clear labels
has
backport
in progress
invalid
kind
breaking
kind
bug
kind
build
kind
dependency
kind
deployment
kind
docs
kind
enhancement
kind
feature
kind
lint
kind
proposal
kind
question
kind
refactor
kind
security
kind
testing
kind
translation
kind
ui
need
backport
priority
critical
priority
low
priority
maybe
priority
medium
reviewed
duplicate
reviewed
invalid
reviewed
wontfix
skip-changelog
status
blocked
status
needs-feedback
status
needs-reviews
status
wip
upstream
gitea
upstream
other
workaround
No Label
Milestone
No items
No Milestone
Assignees
Clear assignees
42wim 6543 a1012112796 Akkuman appleboy axifive betacat0 blumia bwko caicandong ChristopherHX DaanSelen davidsvantesson delvh denyskon fuxiaohei gary-kim garymoon giteabot guillep2k HesterG hiifong jaqra jolheiser jonasfranz jpraet kdumontnu kemzeb kerwin612 khmarbaise KN4CK3R kolaente lafriks lng2020 lotherk luhahn lunny mrsdizzie noerw pat-s puni9869 renovate-bot richmahn rossigee sapk screenshots sillyguodong silverwind smartclip_tim strk tboerger techknowlogick TheFox0x7 tobiasbp typeless volker.raschek wolfogre wxiaoguang yardenshoham yp05327 zeripath Zettat123
No Assignees
3 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: gitea/helm-gitea#550
No description provided.
Delete Branch "TristanHoladay/helm-chart:feature-decouple-rootless"

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?