-
Notifications
You must be signed in to change notification settings - Fork 809
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
Add values.schema.json #2286
base: master
Are you sure you want to change the base?
Add values.schema.json #2286
Conversation
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Code Coverage DiffThis PR does not change the code coverage |
8fd4a86
to
34e7f54
Compare
34e7f54
to
dfe74d9
Compare
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.
One suggested improvement, otherwise PR largely lgtm.
"priorityClassName": { | ||
"description": "Priority class for the Node Daemonset", | ||
"type": [ | ||
"string", | ||
"null" | ||
], | ||
"default": "" | ||
}, |
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.
I was wondering why this diff exists (for the add-on, this section is defined as follows):
"priorityClassName": {
"description": "Priority class for the Node Daemonset",
"type": "string",
"default": "system-node-critical"
},
Which is correct, because that default value is defined in the node template:
priorityClassName: {{ .Values.node.priorityClassName | default "system-node-critical" }} |
but is not currently set in values.yaml
:
priorityClassName: |
I think this would be a good opportunity to fix that discrepancy.
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.
will do !
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.
/lgtm
Largely looks good to me, thank you!
The real test will be going through release process and merging with add-on.
}, | ||
"tag": { | ||
"type": "string", | ||
"default": "v0.5.0" |
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're going to need to update our /hack/release-scripts/generate-sidecar-tags
script to update default sidecar tags like this.
Can you add a backlog task for it or update the script?
}, | ||
"fips": { | ||
"type": "boolean", | ||
"description": "Instruct the AWS SDK to use AWS FIPS endpoints, and deploy container built with BoringCrypto (a FIPS-validated cryptographic library) instead of the Go default. The EBS CSI Driver FIPS images have not undergone FIPS certification, and no official guarnatee is made about the compliance of these images under the FIPS standard. Users relying on these images for FIPS compliance should perform their own independent evaluation", |
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.
There is a typo here for guarantee. Did we run a spellcheck on this file? Your IDE should do this for you (or have a plugin).
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.
Can we run this file through a spellcheck? (Your IDE should do this for you)
What type of PR is this?
/kind feature
What is this PR about? / Why do we need it?
This PR adds the values.schema.json file. Allowing us to make sure changes to helm chart parameters also appear in the eks add-on version of the driver as long as it makes sense to do so.
How was this change tested?
Does this PR introduce a user-facing change?