gitea/helm-chart
48
59
Fork 134
Code Issues 41 Pull Requests 11 Actions Releases 90 Activity

WIP: Add validation schema #198

Closed
justusbunsi wants to merge 20 commits from justusbunsi/helm-chart:feature/92-validation-schema into main
pull from: justusbunsi/helm-chart:feature/92-validation-schema
merge into: gitea:main
Conversation 8 Commits 20 Files Changed 3 +578 -6
justusbunsi commented 2021-06-30 13:37:29 +00:00
Member
Copy Link

⚠️ This is not yet done but more transparent as PR than just being on a random branch on my fork. 😉

And we'll see how the CI can handle it.

Before setting this as non-wip (after everything done):

  • drop first two commits
  • ensure top level "additionalProperties" is set to false
  • consider reverting changes outside of values.schema.json
  • Cleanup helpers.tpl: conditional key handling like enabled in gitea.oauth_settings
⚠️ This is not yet done but more transparent as PR than just being on a random branch on my fork. 😉 And we'll see how the CI can handle it. Before setting this as non-wip (after everything done): - [ ] drop first two commits - [ ] ensure top level "additionalProperties" is set to false - [ ] consider reverting changes outside of values.schema.json - [ ] Cleanup _helpers.tpl_: conditional key handling like _enabled_ in `gitea.oauth_settings`
justusbunsi added this to the Release 4.0.0 milestone 2021-06-30 13:37:29 +00:00
justusbunsi added the
kind
feature
 label 2021-06-30 13:37:29 +00:00
justusbunsi changed title from WIP: Add validate schema to Add validate schema 2021-06-30 14:14:32 +00:00
justusbunsi changed title from Add validate schema to WIP: Add validate schema 2021-06-30 14:14:43 +00:00
justusbunsi force-pushed feature/92-validation-schema from 4bc698e139 to 28d22b6715 2021-06-30 15:36:12 +00:00 Compare
justusbunsi force-pushed feature/92-validation-schema from 28d22b6715 to 33b2890f3a 2021-07-01 16:32:41 +00:00 Compare
justusbunsi changed title from WIP: Add validate schema to WIP: Add validation schema 2021-07-01 16:34:44 +00:00
justusbunsi commented 2021-07-02 15:58:55 +00:00
Author
Member
Copy Link

I think all conditional logic in values.schema.json has to be dropped since Helm supports using/merging multiple files for template rendering. Right now our values.yaml is pretty much a default configuration set. As long as there are options enabled that should be exclusively set (e.g. gitea.admin.existingSecret vs. gitea.admin.username + gitea.admin.password) having such conditional validation seems to stop the entire template rendering due to unmatched conditions.

This would eliminate some really needed validation. ?

I think all conditional logic in _values.schema.json_ has to be dropped since Helm supports using/merging multiple files for template rendering. Right now our values.yaml is pretty much a default configuration set. As long as there are options enabled that should be exclusively set (e.g. `gitea.admin.existingSecret` vs. `gitea.admin.username` + `gitea.admin.password`) having such conditional validation seems to stop the entire template rendering due to unmatched conditions. This would eliminate some really needed validation. ?
justusbunsi added a new dependency 2021-07-02 16:09:31 +00:00
#196 Drop kebab-case configuration notation
justusbunsi removed this from the Release 4.0.0 milestone 2021-07-03 14:42:32 +00:00
justusbunsi removed a dependency 2021-07-03 14:43:24 +00:00
#196 Drop kebab-case configuration notation
justusbunsi force-pushed feature/92-validation-schema from ac154ac9bf to d0e3b24004 2021-09-29 14:19:04 +00:00 Compare
justusbunsi commented 2021-09-29 15:22:00 +00:00
Author
Member
Copy Link

We may need to discuss what's going to be the main purpose of a validation scheme for this Helm Chart. Currently I see various different ways to go and I'd like to discuss them prior further implementation.

  1. Just type safety for Chart related fields
  2. Validate Chart related fields only + type safety by either:
    2.1. Only ensure requirements are fulfilled; no conflict checks
    2.2. Ensure integrity of provided (combined) values sources
  3. All-in validation by either:
    3.1. Way 2.2 + Basic type validation for Kubernetes specific fields
    3.2. Way 3.1 + Basic integrity check for Kubernetes specific fields
    3.3. Way 3.2 + Ensure proper values for Kubernetes specific fields

Some more details for each way:

Way 2.1. would mean: When providing gitea.admin.username you'd need to set gitea.admin.password as well but it does not matter if one of them is missing as soon as you have defined gitea.admin.existingSecret. An provided reference to an existing secret would be always preferred over the inline values.

Way 2.2. would mean: If providing gitea.admin.existingSecret you must not have gitea.admin.username or gitea.admin.password). This could potentially mean that we need to drop default values for conflicting fields in favor of null values to distinguish between defined and non-defined ones. I'm not fully sure if this is really necessary, we'd need to evaluate the behavior of Helm when combining multiple values sources. Nonetheless, this way would prevent configuration constellations to cause templating errors (only for chart related fields) and we might be able to cleanup some checks inside the template files.

Way 3.1. would mean we don't care about consistency of fields passed directly to Kubernetes (e.g. extraVolumeMounts and let the cluster decide whether to like the configuration or not which does not prevent the Helm Chart from being deployed into the cluster.

Way 3.2. would prevent the scenario of way 3.1 since it would not allow you to generate such specifications and apply them to the cluster.

Way 3.3. might be the most strict one but from a Helm Chart's point of view ensures the best possible health for both Helm Chart and the cluster it is running in. It won't let you apply configurations that will not work. On the other hand it would require us to keep track of what's changing in various Kubernetes versions to support a wide version range. Which would be a massive recurring task.

I currently prefer 3.1. (maybe even 3.2.). What do you all think?

We may need to discuss what's going to be the main purpose of a validation scheme for this Helm Chart. Currently I see various different ways to go and I'd like to discuss them prior further implementation. 1. Just _type safety_ for Chart related fields 2. _Validate Chart related_ fields only + _type safety_ by either: 2.1. Only ensure requirements are fulfilled; no conflict checks 2.2. Ensure integrity of provided (combined) values sources 3. _All-in_ validation by either: 3.1. Way 2.2 + Basic type validation for Kubernetes specific fields 3.2. Way 3.1 + Basic integrity check for Kubernetes specific fields 3.3. Way 3.2 + Ensure proper values for Kubernetes specific fields Some more details for each way: Way 2.1. would mean: When providing `gitea.admin.username` you'd need to set `gitea.admin.password` as well but it does not matter if one of them is missing as soon as you have defined `gitea.admin.existingSecret`. An provided reference to an existing secret would be always preferred over the inline values. Way 2.2. would mean: If providing `gitea.admin.existingSecret` you must not have `gitea.admin.username` or `gitea.admin.password`). This could potentially mean that we need to drop default values for conflicting fields in favor of `null` values to distinguish between defined and non-defined ones. I'm not fully sure if this is really necessary, we'd need to evaluate the behavior of Helm when combining multiple values sources. Nonetheless, this way would prevent configuration constellations to cause templating errors (only for chart related fields) and we might be able to cleanup some checks inside the template files. Way 3.1. would mean we don't care about consistency of fields passed directly to Kubernetes (e.g. `extraVolumeMounts` and let the cluster decide whether to like the configuration or not which does not prevent the Helm Chart from being deployed into the cluster. Way 3.2. would prevent the scenario of way 3.1 since it would not allow you to generate such specifications and apply them to the cluster. Way 3.3. might be the most strict one but from a Helm Chart's point of view ensures the best possible health for both Helm Chart and the cluster it is running in. It won't let you apply configurations that will not work. On the other hand it would require us to keep track of what's changing in various Kubernetes versions to support a wide version range. Which would be a massive recurring task. --- I currently prefer 3.1. (maybe even 3.2.). What do you all think?
pat-s commented 2021-09-30 21:52:05 +00:00
Member
Copy Link

Thanks for taking the time to describe your thoughts in such a detail.

I think 3.1 would be great to have as it prevents the user from combining conflicting settings which may lead to unpredictable behaviour (which is hard to debug).

IIUC 3.2 would prevent from setting k8s fields that would not necessarily have an effect on Gitea. I can't really think of a good example here?

I think 2.2 is the main part here as Gitea can be a monster to some degree and it is not always obvious which settings might conflict with each other.

I don't think we should add too much k8s specific validation as the cost/value ratio here is rather small - so my vote goes to 3.1 (which is already a lot of work).

Thanks for taking the time to describe your thoughts in such a detail. I think 3.1 would be great to have as it prevents the user from combining conflicting settings which may lead to unpredictable behaviour (which is hard to debug). IIUC 3.2 would prevent from setting k8s fields that would not necessarily have an effect on Gitea. I can't really think of a good example here? I think 2.2 is the main part here as Gitea can be a monster to some degree and it is not always obvious which settings might conflict with each other. I don't think we should add too much k8s specific validation as the cost/value ratio here is rather small - so my vote goes to 3.1 (which is already a lot of work).
justusbunsi added the
status
blocked
 label 2021-11-13 10:25:44 +00:00
justusbunsi added a new dependency 2021-11-13 10:26:08 +00:00
#241 Drop default values for sensitive and contradictory values
justusbunsi commented 2022-01-27 06:37:23 +00:00
Author
Member
Copy Link

FYI: Helm lint seems to have more verbose output these days.

https://kubelist.com/issue/latest/ and https://www.arthurkoziel.com/validate-helm-chart-values-with-json-schemas/

FYI: Helm lint seems to have more verbose output these days. https://kubelist.com/issue/latest/ and https://www.arthurkoziel.com/validate-helm-chart-values-with-json-schemas/
justusbunsi commented 2022-06-11 17:56:35 +00:00
Author
Member
Copy Link

We use https://github.com/bitnami-labs/readme-generator-for-helm to extract parameters from values.yaml. It also supports generating schema files from it.

I've tested that feature. Unfortunately, we are facing https://github.com/bitnami-labs/readme-generator-for-helm/issues/34. So right now not usable for schema.

We use https://github.com/bitnami-labs/readme-generator-for-helm to extract parameters from `values.yaml`. It also supports generating schema files from it. I've tested that feature. Unfortunately, we are facing https://github.com/bitnami-labs/readme-generator-for-helm/issues/34. So right now not usable for schema.
pat-s commented 2023-04-01 07:32:53 +00:00
Member
Copy Link

@justusbunsi Do you want to continue working on this or shall we close for the moment? Just asking as it's already around for two years.

@justusbunsi Do you want to continue working on this or shall we close for the moment? Just asking as it's already around for two years.
justusbunsi commented 2023-04-01 12:55:07 +00:00
Author
Member
Copy Link

😅 At some point, yes. It will help validating the values. But let's close it for now. I keep the branch on my fork. If anyone want to continue with it before I do, feel free to use the branch as base or inspiration.

😅 At some point, yes. It will help validating the values. But let's close it for now. I keep the branch on my fork. If anyone want to continue with it before I do, feel free to use the branch as base or inspiration.
justusbunsi commented 2023-04-01 12:57:10 +00:00
Author
Member
Copy Link

Apparently I cannot close the PR before closing the blocking issue. But this is still a valid one. So I am removing the dependency from this PR.

Apparently I cannot close the PR before closing the blocking issue. But this is still a valid one. So I am removing the dependency from this PR.
justusbunsi closed this pull request 2023-04-01 12:57:34 +00:00

Pull request closed

Please reopen this pull request to perform a merge.
Sign in to join this conversation.
Reviewers
No reviewers
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
2 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#198
No description provided.
Delete Branch "justusbunsi/helm-chart:feature/92-validation-schema"

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?