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

[ISSUEE 60] Add features to the broker and nameserver #61

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

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