gitea/helm-chart
45
55
Fork 125
Code Issues 32 Pull Requests 8 Actions Releases 87 Activity

Support for SSH log level #358

Merged
pat-s merged 7 commits from pi3ch/helm-chart: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
Contributor
Copy Link

Re #224 (comment)

Re https://gitea.com/gitea/helm-chart/issues/224#issuecomment-717087
pi3ch added 1 commit 2022-09-10 01:11:23 +00:00
Support for SSH log level
All checks were successful
continuous-integration/drone/pr Build is passing
Details
279824e1f1
pat-s approved these changes 2022-09-14 08:49:19 +00:00
pat-s left a comment
Member
Copy Link

Nice catch!

Nice catch!
justusbunsi commented 2022-09-23 13:24:24 +00:00
Member
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 closed this pull request 2022-09-23 13:24:28 +00:00
justusbunsi reopened this pull request 2022-09-23 22:48:44 +00:00
justusbunsi commented 2022-09-23 22:49:12 +00:00
Member
Copy Link

Reopened. See #224 (comment)
Sorry for closing.

Reopened. See https://gitea.com/gitea/helm-chart/issues/224#issuecomment-718042 Sorry for closing.
justusbunsi requested changes 2022-09-25 12:08:48 +00:00
templates/gitea/statefulset.yaml Outdated
@ -210,2 +210,4 @@
- name: SSH_PORT
value: {{ .Values.gitea.config.server.SSH_PORT | quote }}
- name: SSH_LOG_LEVEL
value: {{ .Values.gitea.config.server.SSH_LOG_LEVEL | quote }}
justusbunsi commented 2022-09-25 12:08:36 +00:00
Member
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.
justusbunsi marked this conversation as resolved
pat-s commented 2022-09-25 19:46:32 +00:00
Member
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)?
pi3ch requested review from justusbunsi 2022-09-25 23:27:32 +00:00
pi3ch commented 2022-09-25 23:33:25 +00:00
Author
Contributor
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
Member
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
Member
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
Author
Contributor
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
Member
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 approved these changes 2022-09-27 05:51:13 +00:00
justusbunsi commented 2022-09-27 05:55:06 +00:00
Member
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
Member
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
Member
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
Member
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?
justusbunsi added the
kind
feature
 label 2023-01-14 12:54:40 +00:00
pi3ch commented 2023-01-15 22:20:29 +00:00
Author
Contributor
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 added 1 commit 2023-02-26 14:31:14 +00:00
Merge branch 'main' into main
All checks were successful
continuous-integration/drone/pr Build is passing
Details
35f7e64365
justusbunsi commented 2023-02-26 15:04:14 +00:00
Member
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 added 1 commit 2023-02-26 22:19:22 +00:00
Merge branch 'main' into main
All checks were successful
continuous-integration/drone/pr Build is passing
Details
5b380cfb38
pi3ch commented 2023-02-26 22:21:28 +00:00
Author
Contributor
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
Member
Copy Link

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

@pi3ch Let's do it that way. I've created pi3ch/helm-chart#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 added 2 commits 2023-03-10 22:46:11 +00:00
Move SSH_LOG_LEVEL to ssh related object 
Signed-off-by: justusbunsi <sk.bunsenbrenner@gmail.com>
c0fd66bdd7
pi3ch added 1 commit 2023-03-10 22:47:03 +00:00
Merge branch 'main' into main
All checks were successful
continuous-integration/drone/pr Build is passing
Details
edb1ef7f96
pi3ch commented 2023-03-10 22:47:26 +00:00
Author
Contributor
Copy Link

@justusbunsi done. All good?

@justusbunsi done. All good?
👍 1
justusbunsi approved these changes 2023-03-21 18:21:33 +00:00
justusbunsi left a comment
Member
Copy Link

LGTM. Thanks @pi3ch for your patience.

LGTM. Thanks @pi3ch for your patience.
justusbunsi added 1 commit 2023-03-21 18:21:56 +00:00
Merge branch 'main' into main
All checks were successful
continuous-integration/drone/pr Build is passing
Details
8510cb83c0
justusbunsi commented 2023-03-21 18:23:27 +00:00
Member
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.
pat-s merged commit fdac9e9048 into main 2023-03-22 08:13:31 +00:00
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
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
Milestone
Clear milestone
No items
No Milestone
Assignees
Clear assignees
No Assignees
5 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

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