lunny/helm-chart
1
0
Fork 0
Code Issues 36 Pull Requests 14 Actions Packages Projects Releases 93 Wiki Activity

Add image.fullOverride #550

Merged
TristanHoladay merged 11 commits from feature-decouple-rootless into main 2023-11-14 21:42:27 +00:00
Conversation 4 Commits 11 Files Changed 6 +100 -14
TristanHoladay commented 2023-11-02 15:31:28 +00:00 (Migrated from gitea.com)
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
pat-s commented 2023-11-09 15:37:27 +00:00 (Migrated from gitea.com)
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 commented 2023-11-09 19:43:56 +00:00 (Migrated from gitea.com)
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 (Migrated from gitea.com) reviewed 2023-11-11 17:00:45 +00:00
justusbunsi (Migrated from gitea.com) left a comment
Copy Link

Thanks for continuing the work of #534.

Thanks for continuing the work of #534.
README.md
justusbunsi (Migrated from gitea.com) commented 2023-11-11 17:00:45 +00:00
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.
README.md
justusbunsi (Migrated from gitea.com) commented 2023-11-11 17:40:44 +00:00
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.
README.md
justusbunsi (Migrated from gitea.com) commented 2023-11-11 17:55:30 +00:00
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`.
README.md
justusbunsi (Migrated from gitea.com) commented 2023-11-11 17:58:42 +00:00
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.
README.md
@ -1279,4 +1106,3 @@
```
<!-- prettier-ignore-start -->
<!-- markdownlint-disable-next-line -->
justusbunsi (Migrated from gitea.com) commented 2023-11-11 17:03:22 +00:00
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.
unittests/deployment/image-configuration.yaml
justusbunsi (Migrated from gitea.com) commented 2023-11-11 18:00:45 +00:00
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`.
justusbunsi (Migrated from gitea.com) commented 2023-11-13 14:10:37 +00:00
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 (Migrated from gitea.com) commented 2023-11-13 17:21:32 +00:00
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.
values.yaml
justusbunsi (Migrated from gitea.com) commented 2023-11-11 17:22:49 +00:00
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.
TristanHoladay commented 2023-11-13 14:18:49 +00:00 (Migrated from gitea.com)
Copy Link

Thank you for the awesome feedback and suggestions @justusbunsi.

Thank you for the awesome feedback and suggestions @justusbunsi.
justusbunsi (Migrated from gitea.com) reviewed 2023-11-13 17:33:44 +00:00
README.md
justusbunsi (Migrated from gitea.com) commented 2023-11-13 17:33:44 +00:00
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.
justusbunsi (Migrated from gitea.com) approved these changes 2023-11-14 16:16:03 +00:00
justusbunsi (Migrated from gitea.com) left a comment
Copy Link

Awesome 🚀

Awesome 🚀
justusbunsi commented 2023-11-14 16:59:50 +00:00 (Migrated from gitea.com)
Copy Link

@pat-s Anything left from your side?

@pat-s Anything left from your side?
pat-s (Migrated from gitea.com) approved these changes 2023-11-14 21:42:19 +00:00
pat-s (Migrated from gitea.com) left a comment
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!
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
No Label
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: lunny/helm-chart#550
No description provided.
Delete Branch "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?