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

Support for SSH log level #358

Merged
pi3ch merged 7 commits from main into main 2023-03-22 08:13:31 +00:00
Conversation 18 Commits 7 Files Changed 4 +49
pi3ch commented 2022-09-10 01:11:23 +00:00 (Migrated from gitea.com)
Copy Link

Re https://gitea.com/gitea/helm-chart/issues/224#issuecomment-717087

Re https://gitea.com/gitea/helm-chart/issues/224#issuecomment-717087
pat-s (Migrated from gitea.com) approved these changes 2022-09-14 08:49:19 +00:00
pat-s (Migrated from gitea.com) left a comment
Copy Link

Nice catch!

Nice catch!
justusbunsi commented 2022-09-23 13:24:24 +00:00 (Migrated from gitea.com)
Copy Link

Setting the required environment variable for changing the log level is already possible using the .statefulset.env configuration.

Setting the required environment variable for changing the log level is already possible using the `.statefulset.env` configuration.
justusbunsi commented 2022-09-23 22:49:12 +00:00 (Migrated from gitea.com)
Copy Link

Reopened. See https://gitea.com/gitea/helm-chart/issues/224#issuecomment-718042
Sorry for closing.

Reopened. See https://gitea.com/gitea/helm-chart/issues/224#issuecomment-718042 Sorry for closing.
justusbunsi (Migrated from gitea.com) reviewed 2022-09-25 12:08:36 +00:00
templates/gitea/statefulset.yaml
@ -0,0 +210,4 @@
- name: GITEA_ADMIN_PASSWORD
valueFrom:
secretKeyRef:
key: password
justusbunsi (Migrated from gitea.com) commented 2022-09-25 12:08:36 +00:00
Copy Link

Reviewing the example config file, SSH_LOG_LEVEL seems to be no valid app.ini setting but only an environment variable being used for ssh templating. I suggest either not locating it at .Values.gitea.config.server to prevent this value from being written to the config file, or filtering it out during inline-config processing in _helpers.tpl.

A possible location for this could be a new object .Values.gitea.ssh with key logLevel. Doing so would implicitly ease the way for #321 (disable ssh features).

In any way, a default value must be defined when not checking the existence of a values key.

Reviewing the example config file, `SSH_LOG_LEVEL` seems to be no valid app.ini setting but only an environment variable being used for ssh templating. I suggest either not locating it at `.Values.gitea.config.server` to prevent this value from being written to the config file, or filtering it out during inline-config processing in `_helpers.tpl`. A possible location for this could be a new object `.Values.gitea.ssh` with key `logLevel`. Doing so would implicitly ease the way for #321 (disable ssh features). In any way, a default value must be defined when not checking the existence of a values key.
pat-s commented 2022-09-25 19:46:32 +00:00 (Migrated from gitea.com)
Copy Link

Thanks for checking so thoroughly, @justusbunsi! I did not look at the example app.ini as the naming seemed to make sense inline with the other SSH options.

A possible location for this could be a new object .Values.gitea.ssh with key logLevel. Doing so would implicitly ease the way for #321 (disable ssh features).

This, in combination with a comment why the SSH settings are located in their own block, would be elegant.

@pi3ch Would you be interested in moving into this direction (within this PR)?

Thanks for checking so thoroughly, @justusbunsi! I did not look at the example `app.ini` as the naming seemed to make sense inline with the other SSH options. > A possible location for this could be a new object .Values.gitea.ssh with key logLevel. Doing so would implicitly ease the way for #321 (disable ssh features). This, in combination with a comment why the SSH settings are located in their own block, would be elegant. @pi3ch Would you be interested in moving into this direction (within this PR)?
justusbunsi (Migrated from gitea.com) reviewed 2022-09-25 23:27:32 +00:00
pi3ch commented 2022-09-25 23:33:25 +00:00 (Migrated from gitea.com)
Copy Link

I found all SSH related settings under Server section of the cheatsheet (https://docs.gitea.io/en-us/config-cheat-sheet/#server-server) so it made sense to keep the SSH settings all in once place. Putting it in a difference place could be confusing.

I may suggest Logs section would be more appropriate because firstly there is already another SSH related setting, secondly it is simple for new users to find it out (https://docs.gitea.io/en-us/config-cheat-sheet/#log-log).

I found all SSH related settings under **Server** section of the cheatsheet (https://docs.gitea.io/en-us/config-cheat-sheet/#server-server) so it made sense to keep the SSH settings all in once place. Putting it in a difference place could be confusing. I may suggest **Logs** section would be more appropriate because firstly there is already another SSH related setting, secondly it is simple for new users to find it out (https://docs.gitea.io/en-us/config-cheat-sheet/#log-log).
pat-s commented 2022-09-26 06:31:01 +00:00 (Migrated from gitea.com)
Copy Link

I may suggest Logs section would be more appropriate because firstly there is already another SSH related setting, secondly it is simple for new users to find it out (https://docs.gitea.io/en-us/config-cheat-sheet/#log-log).

Sounds also reasonable. Might be a bit harder to get all SSH-related settings with a on/off switch then if they are split across multiple categories but the LOG_LEVEL one should not make any difference at all if SSH is off anyway.

> I may suggest Logs section would be more appropriate because firstly there is already another SSH related setting, secondly it is simple for new users to find it out (https://docs.gitea.io/en-us/config-cheat-sheet/#log-log). Sounds also reasonable. Might be a bit harder to get all SSH-related settings with a on/off switch then if they are split across multiple categories but the LOG_LEVEL one should not make any difference at all if SSH is off anyway.
justusbunsi commented 2022-09-26 16:58:00 +00:00 (Migrated from gitea.com)
Copy Link

I understand what you mean and it kind of makes sense. Though, I'd still prefer extracting that option into a new object. The whole object .Values.gitea.config simply applies all its content to the app.ini. That's what it's designed for. Without using this Helm Chart you would not add an option to app.ini which has no effect. You would define the environment variable. Even if there are other related options inside app.ini. You know what I mean? It just doesn't feel right adding it there.

With a dedicated object supporting ssh application options and ssh environment options, the chart user wouldn't have to search through the config cheat sheet. The user wouldn't have to configure application options via .Values.gitea.config at all. That would be a feature of this helm chart to simplify ssh configuration as the chart would know what it has to use in which part of the generated helm release.

Surely, supporting all ssh related options at once is totally out of scope of this PR and I don't ask for it. But this PR could lead the way into that direction, even if there is yet another option until it's fully supported.

I understand what you mean and it kind of makes sense. Though, I'd still prefer extracting that option into a new object. The whole object `.Values.gitea.config` simply applies all its content to the app.ini. That's what it's designed for. Without using this Helm Chart you would not add an option to app.ini which has no effect. You would define the environment variable. Even if there are other related options inside app.ini. You know what I mean? It just doesn't feel right adding it there. With a dedicated object supporting ssh application options _and_ ssh environment options, the chart user wouldn't have to search through the config cheat sheet. The user wouldn't have to configure application options via `.Values.gitea.config` at all. That would be a feature of this helm chart to simplify ssh configuration as the chart would know what it has to use in which part of the generated helm release. Surely, supporting all ssh related options at once is totally out of scope of this PR and I don't ask for it. But this PR could lead the way into that direction, even if there is yet another option until it's fully supported.
pi3ch commented 2022-09-26 21:59:59 +00:00 (Migrated from gitea.com)
Copy Link

I personally don't mind and either option is fine with me. Perhaps it is better we cast vote and decide on this so this MR can get in soon.

I personally don't mind and either option is fine with me. Perhaps it is better we cast vote and decide on this so this MR can get in soon.
justusbunsi commented 2022-09-27 04:36:19 +00:00 (Migrated from gitea.com)
Copy Link

Perhaps it is better we cast vote and decide on this so this MR can get in soon.

Good idea. @gitea/Maintainers any opinions on this?

> Perhaps it is better we cast vote and decide on this so this MR can get in soon. Good idea. @gitea/Maintainers any opinions on this?
strk (Migrated from gitea.com) approved these changes 2022-09-27 05:51:13 +00:00
justusbunsi commented 2022-09-27 05:55:06 +00:00 (Migrated from gitea.com)
Copy Link

Anyway, there should be a default value for this setting. Otherwise the environment variable is empty which might cause side effects.

Anyway, there should be a default value for this setting. Otherwise the environment variable is empty which might cause side effects.
pat-s commented 2022-10-03 09:11:57 +00:00 (Migrated from gitea.com)
Copy Link

Let's put things together:

  1. Adding the value at gitea.config.server.SSH_*: Would be in line with existing SSH settings but the helm chart would be somewhat off as it would add a setting to gitea.config.server which does not exist in Gitea itself
  2. A new section at gitea.ssh in the helm chart.
  3. Adding the value to statefulset.env and treating is as a normal env var which is picked up by some service internally.

(1) Only makes sense if the setting would be supported by Gitea itself, as Justus pointed out. Given all the other SSH options existing there, putting the new one there would help with consistency. However, this would require a change in Gitea core and not result in a quick solution to the problem.
(2) Might be cleaner and quicker than (1) in the first place but needs clear documentation stating why this one SSH setting lives there and not in gitea.config.server.
(3) Is more like an option for additional, user-based env vars which should be injected additionally.

A hybrid solution could also be to start with (2) for a quick fix and aim for (1) longterm, i.e. trying to get SSH_LOG_LEVEL to be an official option in Gitea so the helm chart can define it in gitea.config.server?

Let's put things together: 1. Adding the value at `gitea.config.server.SSH_*`: Would be in line with existing SSH settings but the helm chart would be somewhat off as it would add a setting to `gitea.config.server` which does not exist in Gitea itself 2. A new section at `gitea.ssh` in the helm chart. 3. Adding the value to `statefulset.env` and treating is as a normal env var which is picked up by some service internally. (1) Only makes sense if the setting would be supported by Gitea itself, as Justus pointed out. Given all the other SSH options existing there, putting the new one there would help with consistency. However, this would require a change in Gitea core and not result in a quick solution to the problem. (2) Might be cleaner and quicker than (1) in the first place but needs clear documentation stating why this one SSH setting lives there and not in `gitea.config.server`. (3) Is more like an option for additional, user-based env vars which should be injected additionally. A hybrid solution could also be to start with (2) for a quick fix and aim for (1) longterm, i.e. trying to get `SSH_LOG_LEVEL` to be an official option in Gitea so the helm chart can define it in `gitea.config.server`?
luhahn commented 2022-10-06 07:32:19 +00:00 (Migrated from gitea.com)
Copy Link

I agree with @justusbunsi it doesn't really feel right to add it to gitea.config
therefore I also vote for creating a dedicated ssh object for all ssh related settings.
gitea.ssh seems to be fitting here :)

I agree with @justusbunsi it doesn't really feel right to add it to `gitea.config` therefore I also vote for creating a dedicated ssh object for all ssh related settings. `gitea.ssh` seems to be fitting here :)
justusbunsi commented 2023-01-14 12:51:30 +00:00 (Migrated from gitea.com)
Copy Link

@pi3ch With regards to Chart maintainability I would like to prevent a quick fix resulting in a breaking change or at least inconsistency when adding related features such as #321.

Do you want to continue with this PR or do you mind granting write permissions to maintainers via checkbox on the right?

@pi3ch With regards to Chart maintainability I would like to prevent a quick fix resulting in a breaking change or at least inconsistency when adding related features such as #321. Do you want to continue with this PR or do you mind granting write permissions to maintainers via checkbox on the right?
pi3ch commented 2023-01-15 22:20:29 +00:00 (Migrated from gitea.com)
Copy Link

@pi3ch With regards to Chart maintainability I would like to prevent a quick fix resulting in a breaking change or at least inconsistency when adding related features such as #321.

Do you want to continue with this PR or do you mind granting write permissions to maintainers via checkbox on the right?

Sure. Granted you the access.

> @pi3ch With regards to Chart maintainability I would like to prevent a quick fix resulting in a breaking change or at least inconsistency when adding related features such as #321. > > Do you want to continue with this PR or do you mind granting write permissions to maintainers via checkbox on the right? Sure. Granted you the access.
👍 1
justusbunsi commented 2023-02-26 15:04:14 +00:00 (Migrated from gitea.com)
Copy Link

@pi3ch With regards to Chart maintainability I would like to prevent a quick fix resulting in a breaking change or at least inconsistency when adding related features such as #321.

Do you want to continue with this PR or do you mind granting write permissions to maintainers via checkbox on the right?

Sure. Granted you the access.

@pi3ch, I thought using the checkbox on the right would also grant write permissions to your fork branch. Using "Update branch by merge" button worked but pushing from command line is rejected with "Unauthorized" message. Do you have some sort of branch protection configured? Do you mind granting me explicit write permissions to the PR branch? Or shall I create a Pull Request towards your repository?

> > @pi3ch With regards to Chart maintainability I would like to prevent a quick fix resulting in a breaking change or at least inconsistency when adding related features such as #321. > > > > Do you want to continue with this PR or do you mind granting write permissions to maintainers via checkbox on the right? > > Sure. Granted you the access. @pi3ch, I thought using the checkbox on the right would also grant write permissions to your fork branch. Using "Update branch by merge" button worked but pushing from command line is rejected with "Unauthorized" message. Do you have some sort of branch protection configured? Do you mind granting me explicit write permissions to the PR branch? Or shall I create a Pull Request towards your repository?
pi3ch commented 2023-02-26 22:21:28 +00:00 (Migrated from gitea.com)
Copy Link

@justusbunsi Gitea UI is all new to me. where can I find exclusive write PR? I've just pressed "Update by merge", unsure that added your changes.

I didn't setup any branch protection, was just a vanilla PR.

You can try to PR to my branch and I MR here.

@justusbunsi Gitea UI is all new to me. where can I find exclusive write PR? I've just pressed "Update by merge", unsure that added your changes. I didn't setup any branch protection, was just a vanilla PR. You can try to PR to my branch and I MR here.
justusbunsi commented 2023-03-07 16:06:23 +00:00 (Migrated from gitea.com)
Copy Link

You can try to PR to my branch and I MR here.

@pi3ch Let's do it that way. I've created https://gitea.com/pi3ch/helm-chart/pulls/1 to your PR branch.

> You can try to PR to my branch and I MR here. @pi3ch Let's do it that way. I've created https://gitea.com/pi3ch/helm-chart/pulls/1 to your PR branch.
pi3ch commented 2023-03-10 22:47:26 +00:00 (Migrated from gitea.com)
Copy Link

@justusbunsi done. All good?

@justusbunsi done. All good?
👍 1
justusbunsi (Migrated from gitea.com) approved these changes 2023-03-21 18:21:33 +00:00
justusbunsi (Migrated from gitea.com) left a comment
Copy Link

LGTM. Thanks @pi3ch for your patience.

LGTM. Thanks @pi3ch for your patience.
justusbunsi commented 2023-03-21 18:23:27 +00:00 (Migrated from gitea.com)
Copy Link

@pat-s Do you agree with what I changed? If so, we can merge it.

@pat-s Do you agree with what I changed? If so, we can merge it.
Sign in to join this conversation.
Reviewers
No Reviewers
pat-s
strk
justusbunsi
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
kind/feature
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#358
No description provided.
Delete Branch "main"

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?