-
Notifications
You must be signed in to change notification settings - Fork 29
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
values schema #811
values schema #811
Conversation
@d7oc could you skim those changes to check if you agree with the principle of this change? Most interesting from my side:
If we can agree on the schema beeing a good thing, I'll gonna do some polishing on this PR, especially on the configuration documentation that received a small fallout. |
34c49fa
to
06f3d2b
Compare
@d7oc I'd like to get this into the chart soonish or agree on a timeframe when this change does fits our risk analysis / schedule / ... This week I had another occurence where this schema feature would have saved a college and my like 2 hours each because we had fine looking configuration on a wrong indention level. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Submitting first feedback. Check until line 800 in values.yaml
I'm missing the command here to generate the schema from the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Finished review.
charts/ocis/values.yaml
Outdated
# @schema | ||
# type: [string, null] | ||
# required: false | ||
# @schema |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could use format: "uri"
here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
charts/ocis/values.yaml
Outdated
# @schema | ||
# type: [string, null] | ||
# required: false | ||
# @schema |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could use format: "uri"
here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
charts/ocis/values.yaml
Outdated
# @schema | ||
# type: [string, null] | ||
# required: false | ||
# @schema |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could use format: "uri"
here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
charts/ocis/values.yaml
Outdated
# -- Configure the {"styles": []} section in the Web config.json. | ||
styles: | ||
[] | ||
# - href: /theme/foo.css | ||
# @schema | ||
# additionalProperties: true | ||
# @schema | ||
# -- Configure the {"styles": []} section in the Web config.json. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems to me that this description of the setting is wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
charts/ocis/values.yaml
Outdated
postLogoutRedirectURI: | ||
# @schema | ||
# type: [string, null] | ||
# required: false | ||
# @schema |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could add format: "uri"
here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
charts/ocis/values.yaml
Outdated
# @schema | ||
# type: [string, null] | ||
# required: false | ||
# @schema |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could add format: "uri"
here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
Description
Maintain a schema file to find misconfigurations.
There were already some misconfigurations found in the deployment examples (see the diff).
Related Issue
Motivation and Context
How Has This Been Tested?
helm lint charts/ocis -f ~/Projects/gitlab.xxx/exxx/ocis-infra/common/ocis/values.yaml -f ~/Projects/gitlab.xxx/exxx/ocis-infra/prod/apps/ocis/values.yaml
Screenshots (if appropriate):
Types of changes
Checklist:
make docs
) and committed