Skip to content
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

docs: add jsonschema for values.yaml #458

Open
wants to merge 9 commits into
base: develop
Choose a base branch
from
Open

Conversation

annekebr
Copy link
Collaborator

@annekebr annekebr commented Dec 17, 2021

Adding a jsonschema allows user to fail early as the user configuration in the is validated at installation time already. For that reason, there has been added additional validation that couldn't be performed using jsonschema in the helm helper file. Moreover, as we now have our dear conny in artifacthub we can enhance our artifacthub page by adding a jsonschema as artifacthub parses the file and shows its content in a nice format.

Fixes #

Description

Checklist

  • PR is rebased to/aimed at branch develop
  • PR follows Contributing Guide
  • Added tests (if necessary)
  • Extended README/Documentation (if necessary)
  • Adjusted versions of image and Helm chart in values.yaml and Chart.yaml (if necessary)

@annekebr annekebr force-pushed the docs/add-jsonschema branch from 39d7652 to f7628e1 Compare December 17, 2021 16:50
@annekebr annekebr marked this pull request as ready for review December 17, 2021 16:50
@annekebr annekebr changed the base branch from master to develop December 17, 2021 16:50
.github/PULL_REQUEST_TEMPLATE.md Outdated Show resolved Hide resolved
helm/values.schema.json Outdated Show resolved Hide resolved
helm/values.schema.json Outdated Show resolved Hide resolved
helm/values.schema.json Outdated Show resolved Hide resolved
helm/values.schema.json Outdated Show resolved Hide resolved
helm/values.schema.json Outdated Show resolved Hide resolved
helm/values.schema.json Outdated Show resolved Hide resolved
helm/values.schema.json Show resolved Hide resolved
helm/values.schema.json Outdated Show resolved Hide resolved
helm/values.schema.json Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented Jan 14, 2022

Codecov Report

Merging #458 (f5921ab) into develop (c85c4bb) will decrease coverage by 0.15%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #458      +/-   ##
===========================================
- Coverage    94.40%   94.24%   -0.16%     
===========================================
  Files           22       22              
  Lines         1125     1129       +4     
===========================================
+ Hits          1062     1064       +2     
- Misses          63       65       +2     
Impacted Files Coverage Δ
connaisseur/logging_wrapper.py 0.00% <0.00%> (ø)
connaisseur/workload_object.py 98.24% <0.00%> (ø)
connaisseur/validators/cosign/cosign_validator.py 97.80% <0.00%> (+0.04%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c85c4bb...f5921ab. Read the comment docs.

@annekebr annekebr force-pushed the docs/add-jsonschema branch 2 times, most recently from 0f44808 to 4fb231e Compare January 21, 2022 14:56
@annekebr annekebr requested a review from Starkteetje January 21, 2022 15:02
@annekebr annekebr dismissed Starkteetje’s stale review January 21, 2022 15:02

Implemented suggested changes

helm/values.schema.json Outdated Show resolved Hide resolved
helm/values.schema.json Outdated Show resolved Hide resolved
helm/values.schema.json Outdated Show resolved Hide resolved
helm/values.schema.json Outdated Show resolved Hide resolved
helm/values.schema.json Outdated Show resolved Hide resolved
helm/values.schema.json Outdated Show resolved Hide resolved
helm/values.schema.json Outdated Show resolved Hide resolved
Starkteetje
Starkteetje previously approved these changes Jan 21, 2022
@Starkteetje
Copy link
Member

remaining nit: it's not really a docs change, so probably commit msg should be changed, otherwise lgtm

@xopham xopham mentioned this pull request Feb 4, 2022
4 tasks
@annekebr
Copy link
Collaborator Author

@Starkteetje yeah, I was a bit torn in choosing the commit msg. Would you prefer refactor? I don't think there is a 💯 fitting msg, do you?

@annekebr annekebr force-pushed the docs/add-jsonschema branch from b9dc10b to 7edc099 Compare February 11, 2022 07:59
@xopham xopham mentioned this pull request Feb 11, 2022
6 tasks
@annekebr annekebr force-pushed the docs/add-jsonschema branch from 3f4dbc7 to 220b645 Compare February 11, 2022 20:09
Adding a jsonschema allows user to fail early as the user configuration in the  is validated at installation time already. For that reason, there has been added additional validation that couldn't be performed using jsonschema in the helm helper file. Moreover, as we now have our dear conny in artifacthub we can enhance our artifacthub page by adding a jsonschema as artifacthub parses the file and shows its content in a nice format.
@annekebr annekebr force-pushed the docs/add-jsonschema branch from 220b645 to 655706f Compare February 14, 2022 10:49
Starkteetje
Starkteetje previously approved these changes Feb 18, 2022
@xopham
Copy link
Collaborator

xopham commented Feb 20, 2022

@annekebr is this feature bound to be merged soon or will changes such as in #551 and #428 have to be added to this PR?

helm/values.schema.json Outdated Show resolved Hide resolved
helm/values.schema.json Outdated Show resolved Hide resolved
"if": {
"properties": {
"type": {
"const": "cosign"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have tried to violate any of the schema rules below and could not get an error for that via helm lint. Is that expected or something off?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there was sth off - thanks!! It should work now (excited for the integration tests 👯 😁)

Comment on lines 273 to 280
"imagePullPolicy",
"resources",
"nodeSelector",
"tolerations",
"affinity",
"securityContext",
"podSecurityPolicy",
"envs"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are those actually required? bc I believe at least some of them can be dropped and will have some type of defaults, isn't it?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but I am not sure which policy was decided upon

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no, you're right - these are not actually required, but we don't set any kind of default (but k8s does) and we once talked about it and decided to require these values for that reason, but rethinking it, I changed my mind and we should not require them if leaving them out does not yield a non-functional installation, what do you think?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed the non-required values here and made the defaults explicit where I found defaults - if you don't like it I can change it back :)

Copy link
Collaborator

@xopham xopham left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do have a few comments. Most notably, none of the things I do to violate the validator schemas seems to fail

Comment on lines 273 to 280
"imagePullPolicy",
"resources",
"nodeSelector",
"tolerations",
"affinity",
"securityContext",
"podSecurityPolicy",
"envs"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but I am not sure which policy was decided upon

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants