-
Notifications
You must be signed in to change notification settings - Fork 17
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
feat: add tolerations and affinity in the global values #476
Conversation
Adds two new global configuration fields to allow users to define the affinity and tolerations to be used in the policy server and controller deployments as well as audit scanner pods. Signed-off-by: José Guilherme Vanz <jguilhermevanz@suse.com>
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, but I spotted something unrelated with the PR that might be broken. See comments
@@ -47,3 +49,5 @@ global: | |||
default: | |||
name: default | |||
enabled: true | |||
affinity: {} |
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.
(opening thread randomly here)
It seems that the CI job for make check-common-values
is not working, possibly because we are using the wrong yq
dependency (see here).
It would have catched the discrepancy with common-values.yaml and each of the other values.yaml.
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.
Oh, I was taking a look in the CI history. That's failing for a while already. =(
I guess the ubuntu runner is running the go lang version. But we are using the python command in the script
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'm opening an issue for this bug. Just to avoid mixing context here.
@@ -3,6 +3,61 @@ | |||
# by more than one chart and they ideally need to match during the | |||
# installation of the charts consuming this values. | |||
global: | |||
# affinity: |
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 believe that instead of having the repetition on this commented configuration, this should go to common-values.yaml
, and be checked that way. It may be needed to edit the make check-common-values
as needed.
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've added the examples in the charts values because that is sent in the helm chart bundle. Which helps users to get examples of how to configure the helm charts. For example, Artifacthub allow user to visualize the default values. If we add the comments in the common-values.yaml
the users will not see an example of how to use the fields.
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'm fine with that, but the make check-common-values
CI job would have failed. I would also add them into the common-values.yaml, and if the make works, it will check that they are all in sync at least.
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.
The CI failed for another reason. The CI is failing because the script is using a CLI flag not available in the yq
installed in the github runner. There is a yq
implementation in go lang and another one using python (the one with the required CLI flag). The script is considering one yq
but the runner has another installed. Which misses the '--sort-keys' CLI flag. We need to update the script to remove the need of --sort-keys
or install the right yq
version.
The CI is failing for a while now. If you check the CI history they are green. But if you read the logs, the script failed with the same problem. I've opened another issue to attack this problem. It seems out of scope of this PR. I've run the make check-common-values
locally and it's passing. I have the python yq
installed and it does not consider comment when comparing the values:
~/SUSE/helm-charts │ issue756 !1 ?1 make check-common-values ✔ │ 11:46:13
~/SUSE/helm-charts │ issue756 !1 ?1 ✔ │ 14:09:58
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 have the python yq installed and it does not consider comment when comparing the values
fair enough then, approving.
The nodeSelector configuration was defined in the container. This is wrong, this configuration should be in the PodSpec. This commit moves this configuration there. Signed-off-by: José Guilherme Vanz <jguilhermevanz@suse.com>
Description
Add two new fields in the global values used by the Kubewarden helm charts to add affinity and tolerations configuration in all the Helm charts.
Fix kubewarden/kubewarden-controller#756