-
Notifications
You must be signed in to change notification settings - Fork 46
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
Configurable Allocation API Port #419
Conversation
@nottagg thanks for the PR and sorry we haven't yet taken a look at it. Promise that I'll do it soon, thanks for understanding! |
@nottagg apologies again for the delay. I really am not a fan of injecting both the integer and the string form of the port, have spent all day trying to find an alternative. |
Now that I think about it though, an alternative solution would be NOT to define the port in the Makefile. We can do something like that on the # rest of YAML file...
- name: ALLOC_API_SVC_PORT
value: "5000"
- name: TLS_SECRET_NAMESPACE
valueFrom:
fieldRef:
fieldPath: metadata.namespace
name: manager
securityContext:
allowPrivilegeEscalation: false
livenessProbe:
httpGet:
path: /healthz
port: 8081
initialDelaySeconds: 15
periodSeconds: 20
readinessProbe:
httpGet:
path: /readyz
port: 8081
initialDelaySeconds: 5
periodSeconds: 10
resources:
requests:
cpu: 100m
memory: 500Mi
limits:
cpu: 1000m
memory: 500Mi
ports:
- containerPort: 5000
hostPort: 5000
# rest of YAML file... People that use the port 5000 can manually change it on this file. Agreed, it's more cumbersome than customizing directly but I think it partially solves the problem. Can we do it this way? Left some more comments as well. Thanks again! |
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.
Let me know if you have any questions, thanks!
Hi Dimitris. No problem with the delay! I'm just happy to be a part of a OSS project I thoroughly enjoy. Thanks for all of the feedback! |
thank you!
|
ping @nottagg, we'd like to push 0.6 next week and we'd love to include your changes! |
hey @nottagg, thanks again for working on this! Let me know when it's ready to review again |
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.
thanks @nottagg! |
fixes #379
Did not get this in time for the hackathon so I'll have to settle with it being my first real OSS PR instead. Never worked with Go or K8s so if there's some unclean code, I can adjust it as needed.
Changes:
Kustomize is moving away from env variables (see - kubernetes-sigs/kustomize#3485 ) and there was an issue with kustomize removing double quotes when replacing variables in the yaml. The workaround in this PR is sending the env variable as both a string and integer (9eae5ff). It might be worth exploring revamping the kustomization.yaml files in the future.