-
Notifications
You must be signed in to change notification settings - Fork 232
[OpenShift] fixes the disable affinity assistant configuration #377
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
Conversation
This is always true in case of openshift due to which user would not able to enable it. so this fixes by removing the defaulting in Operator CR and adding the field only when user adds it. By default, the value in configmap is false for kubernetes and will be changed to true for openshift which user can change if requires. Signed-off-by: Shivam Mukhade <smukhade@redhat.com>
| *properties.DisableAffinityAssistant = DefaultDisableAffinityAssistant | ||
| // Set `disable-affinity-assistant` to true if not set in CR | ||
| // webhook will not set any value but by default in pipelines configmap it will be false | ||
| if properties.DisableAffinityAssistant == nil { |
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.
so from Operator we can configure DisableAffinityAssistant only for Openshift and not for K8s then in that case can we add comment here https://github.com/tektoncd/operator/blob/main/pkg/apis/operator/v1alpha1/tektonpipeline_types.go#L84 so that it will be more clear
not necessarily in this instead we do it in another PR
WDYT?
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.
@savitaashture it's support on all platform. Only on Openshift the default is different (true instead of false)
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.
yep. on kubernetes by default the field will not be set in TektonConfig CR, user can define and alter value.
by default it will be false in configmap.
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.
@sm43 but on k8s, if the user set then in the TektonConfig CR, the will be applied to the managed tekton instance right ?
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.
yes.. user can define the field in k8s and change the value
|
/lgtm /hold |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: vdemeester The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/hold cancel |
|
/lgtm |
This is always true in case of openshift due to which user would
not able to enable it. so this fixes by removing the defaulting in
Operator CR and adding the field only when user adds it.
By default, the value in configmap is false for kubernetes and will
be changed to true for openshift which user can change if requires.
fixes #372
Signed-off-by: Shivam Mukhade smukhade@redhat.com
Changes
Submitter Checklist
These are the criteria that every PR should meet, please check them off as you
review them:
See the contribution guide for more details.
Release Notes