-
Notifications
You must be signed in to change notification settings - Fork 21
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
EnvoyFleet customization improvements #143
Conversation
Perhaps we should open another issue for this @Tarick ? |
Will you update the docs? |
sharedLabels map[string]string | ||
} | ||
|
||
func NewEnvoyFleetResources(ctx context.Context, client client.Client, ef *gatewayv1alpha1.EnvoyFleet) (*EnvoyFleetResources, error) { |
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.
Im not sure about this one
Its doing a lot. I feel it would be better to have separate functions for creating each of the required resources. For transparency more than anything
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 problem is that some of those resources depend on others. Configmap is separate, but Deployment uses this configmap, Service uses deployment labels selector - so we need to create specific variables and share between these. Thus the struct, thus the specific order too in NewEnvoyFleetResources.
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.
To specify dependencies, you specify parameters to the functions that depend on other things
configMap := createConfig()
deployment := createDeployment(configMap)
svc := createService(deployment)
For example ☝️
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.
or use Class :)
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.
@dobegor what do you think?
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.
For me this is not a deal breaker for approving your PR. Just a personal opinion of mine. Im curious to find out Egor's opinion
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 wish these functions would return objects that they generate and the final thing would be composed of them here rather than having everything done in a procedural style... However we can still pull this in and refactor it afterwards.
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 agree with Kyle's comments and I'd like to refactor the resource generation bits but I think it's already big enough to merge it 😃
This PR closes #133
Changes
-- Scheduler hints (NodeSelector, Tollerations, Affinity/Antiaffinity)
-- CPU and Memory Resources (limits and requests)
-- setting custom public Envoy Image
-- Adding custom annotations (for e.g. Prometheus scrapping)
-- Service customization - the ability to set ports, type and annotations to fully support ingress Service in any environment
kubectl describe envoyfleet default
. Still requires more improvements since technically Service, Deployment and Config map statuses are not checked.As part of the implementation, local development environment was reworked to support the same labels and resources names as in the Helm chart and subsequently
Updates
Follow up items:
These tasks needs to be done just before we tag the new version:
main
branch and it will be available immediately while must be visible only when we make the new tag and Helm chart.Follow up issue will be created.
Fixes
Checklist