Skip to content

Conversation

linjiemiao
Copy link

  1. add affinity / securityContext / imagePullSecrets / tolerations / nodeSelector / podAnnotations / priorityClassName

   nodeSelector / podAnnotations / priorityClassName
Comment on lines +407 to +412
Affinity: broker.Spec.Affinity,
SecurityContext: broker.Spec.SecurityContext,
ImagePullSecrets: broker.Spec.ImagePullSecrets,
Tolerations: broker.Spec.Tolerations,
NodeSelector: broker.Spec.NodeSelector,
PriorityClassName: broker.Spec.PriorityClassName,
Copy link

@AdheipSingh AdheipSingh Nov 15, 2020

Choose a reason for hiding this comment

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

You need to write functions for each of them and handle nil value scenarios, by directly adding this you are forcing the user at all times to specify these values, the operator will crash if you don't pass in tolerations.
@liuruiyiyang since these values will go to both broker/nameserver we can create single functions which can be used both in broker and nameserver

Copy link
Contributor

Choose a reason for hiding this comment

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

+1 This pr added features that help enrich operator, hope that corresponding null check would be supplemented.

Comment on lines +324 to +329
Affinity: nameService.Spec.Affinity,
SecurityContext: nameService.Spec.SecurityContext,
ImagePullSecrets: nameService.Spec.ImagePullSecrets,
Tolerations: nameService.Spec.Tolerations,
NodeSelector: nameService.Spec.NodeSelector,
PriorityClassName: nameService.Spec.PriorityClassName,

Choose a reason for hiding this comment

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

same here, we need to write functions to get values to handle nil/empty scenarios

Comment on lines +407 to +412
Affinity: broker.Spec.Affinity,
SecurityContext: broker.Spec.SecurityContext,
ImagePullSecrets: broker.Spec.ImagePullSecrets,
Tolerations: broker.Spec.Tolerations,
NodeSelector: broker.Spec.NodeSelector,
PriorityClassName: broker.Spec.PriorityClassName,
Copy link
Contributor

Choose a reason for hiding this comment

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

+1 This pr added features that help enrich operator, hope that corresponding null check would be supplemented.

Comment on lines +61 to +65
NodeSelector map[string]string `json:"nodeSelector,omitempty"`
// PodAnnotations you can use annotations to attach arbitrary non-identifying metadata to objects.
PodAnnotations map[string]string `json:"podAnnotations,omitempty"`
// PriorityClassName defines priority class's name
PriorityClassName string `json:"priorityClassName,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems null checks are required for them, would define them as pointer be better?

Comment on lines +68 to +72
NodeSelector map[string]string `json:"nodeSelector,omitempty"`
// PodAnnotations you can use annotations to attach arbitrary non-identifying metadata to objects.
PodAnnotations map[string]string `json:"podAnnotations,omitempty"`
// PriorityClassName defines priority class's name
PriorityClassName string `json:"priorityClassName,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems null checks are required for them, would define them as pointer be better?

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.

3 participants