gitea/helm-chart
45
54
Fork 128
Code Issues 31 Pull Requests 7 Actions Releases 87 Activity

Rewrite init script #178

Merged
techknowlogick merged 14 commits from justusbunsi/helm-chart:rewrite-init-scripts into master 2021-06-29 20:09:16 +00:00
Conversation 4 Commits 14 Files Changed 4 +130 -41
justusbunsi commented 2021-06-12 18:32:57 +00:00
Member
Copy Link

These changes rewrite the init script to be error aware, informative and have a bit more security awareness.

During rewrite several hidden bugs could be identified and fixed, such as:

  • LDAP configuration options interpreted by the shell before passed to command
  • Finding multiple ldap ids instead of one during lookup when their names are almost identical
    e.g. _my-ldap-auth and my-ldap-auth
  • Properly filter auth sources by their types to prevent unintended type converting attempts that fail

In addition to that the script is a bit cleaner. Some commands do not exist anymore and would cause false-positive errors during script execution.

Helps for: #149

These changes rewrite the init script to be error aware, informative and have a bit more security awareness. During rewrite several hidden bugs could be identified and fixed, such as: - LDAP configuration options interpreted by the shell before passed to command - Finding multiple ldap ids instead of one during lookup when their names are almost identical e.g. `_my-ldap-auth` and `my-ldap-auth` - Properly filter auth sources by their types to prevent unintended type converting attempts that fail In addition to that the script is a bit cleaner. Some commands do not exist anymore and would cause false-positive errors during script execution. Helps for: #149
justusbunsi commented 2021-06-12 18:35:51 +00:00
Author
Member
Copy Link

I've tested all changes for every possibility I could think of (probably not all ?). Even switching between both available images (root, rootless) to verify interoperability.

Some things (not really related to my changes) didn't work (properly):

Using not-active, skip-tls-verify, allow-deactivate-all, synchronize-users and attributes-in-bind for LDAP configuration. They are boolean values and would be passed like --not-active '' which breaks the command for some reason.
Same for use-custom-urls for OAuth command. (Edit: incorrect statement)

Should I open separate issues for this or do we fix boolean option passing in this PR as well since they would break the init script? (Edit: Yes, fix in this PR as well.)

I've tested all changes for every possibility I could think of (probably not all ?). Even switching between both available images (root, rootless) to verify interoperability. Some things (not really related to my changes) didn't work (properly): Using _not-active_, _skip-tls-verify_, _allow-deactivate-all_, _synchronize-users_ and _attributes-in-bind_ for LDAP configuration. They are boolean values and would be passed like `--not-active ''` which breaks the command for some reason. ~~Same for `use-custom-urls` for OAuth command.~~ (Edit: incorrect statement) ~~Should I open separate issues for this or do we fix boolean option passing in this PR as well since they would break the init script?~~ (Edit: Yes, fix in this PR as well.)
justusbunsi reviewed 2021-06-12 18:37:38 +00:00
templates/gitea/statefulset.yaml Outdated
@ -42,0 +49,4 @@
- name: GITEA_WORK_DIR
value: /data
- name: GITEA_TEMP
value: /tmp/gitea
justusbunsi commented 2021-06-12 18:37:38 +00:00
Author
Member
Copy Link

Should we include .Values.statefulset.env into init-directories init container since initPreScript execution takes place here and someone could reference custom environment variables?

Should we include `.Values.statefulset.env` into `init-directories` init container since `initPreScript` execution takes place here and someone could reference custom environment variables?
justusbunsi marked this conversation as resolved
justusbunsi commented 2021-06-14 17:06:40 +00:00
Author
Member
Copy Link

I just re-read the issue #149 and am sure that this PR will not fully fix it. The repeated failure on init container due to unavailable db is done and the script is more verbose. But right now no credentials are used for the db check.

I just re-read the issue #149 and am sure that this PR will not fully fix it. The repeated failure on init container due to unavailable db is done and the script is more verbose. But right now no credentials are used for the db check.
luhahn commented 2021-06-15 20:30:17 +00:00
Member
Copy Link

really like what you did here, will test this in a few days

really like what you did here, will test this in a few days
justusbunsi added a new dependency 2021-06-19 14:10:28 +00:00
#183 Fix admin + ldap configuration
justusbunsi changed title from Rewrite init script to WIP: Rewrite init script 2021-06-19 14:17:07 +00:00
justusbunsi added the
kind
enhancement
 label 2021-06-19 14:19:10 +00:00
justusbunsi force-pushed rewrite-init-scripts from 6ffdb5c938 to fda4d86b87 2021-06-21 16:48:55 +00:00 Compare
justusbunsi removed a dependency 2021-06-21 16:49:27 +00:00
#183 Fix admin + ldap configuration
justusbunsi changed title from WIP: Rewrite init script to Rewrite init script 2021-06-21 16:49:36 +00:00
justusbunsi force-pushed rewrite-init-scripts from fda4d86b87 to 805e863a63 2021-06-25 08:12:21 +00:00 Compare
justusbunsi added the
kind
refactor
 label 2021-06-26 11:35:11 +00:00
luhahn approved these changes 2021-06-29 08:22:34 +00:00
luhahn left a comment
Member
Copy Link

Tested this PR in different clusters, with existing PVCs and new PVCs.
Looks good :)

Tested this PR in different clusters, with existing PVCs and new PVCs. Looks good :)
justusbunsi changed title from Rewrite init script to WIP: Rewrite init script 2021-06-29 14:48:38 +00:00
justusbunsi changed title from WIP: Rewrite init script to Rewrite init script 2021-06-29 18:49:55 +00:00
justusbunsi commented 2021-06-29 18:50:58 +00:00
Author
Member
Copy Link

@luhahn Done. Ready for another review.

@luhahn Done. Ready for another review.
justusbunsi force-pushed rewrite-init-scripts from b66f76b571 to 41ed6f66dc 2021-06-29 19:37:46 +00:00 Compare
techknowlogick approved these changes 2021-06-29 20:09:08 +00:00
techknowlogick merged commit 9059229acb into master 2021-06-29 20:09:16 +00:00
justusbunsi deleted branch rewrite-init-scripts 2021-06-29 20:11:28 +00:00
justusbunsi added this to the Release 4.0.0 milestone 2021-06-30 10:14:34 +00:00
Sign in to join this conversation.
Reviewers
No reviewers
luhahn
techknowlogick
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
Release 4.0.0
Assignees
Clear assignees
No Assignees
3 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#178
No description provided.
Delete Branch "justusbunsi/helm-chart:rewrite-init-scripts"

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?