-
Notifications
You must be signed in to change notification settings - Fork 57
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
chore: support Prefix type in ingress path. #35
Conversation
Use pathType as Prefix to work with Application Load Balancer.
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 for the PR! Just needs one small change
Also, can you bump the chart version to 3.7.0
?
@@ -50,7 +50,7 @@ spec: | |||
{{- end }} | |||
port: | |||
number: {{ default $servicePort .port }} | |||
pathType: ImplementationSpecific | |||
pathType: Prefix |
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 avoid any breaking changes, can we pull this out into a variable and default to ImplementationSpecific
?
Hello @b0nete if you are busy, I can pick it up from where you stopped and start working on it. I will be glad to do it. thank you |
I am going to take over this PR and submit it for merging this week |
Signed-off-by: Federico Luna Agüero <federico@craftech.io>
bump: chart version up to 3.7.0
@nerdeveloper can you merge your PR #42 in this branch? |
@cbuto chart version was upgraded to 3.7.0 |
unfortunately, I am not sure I can, as I dont have access to this particular branch |
sure, @b0nete or @cbuto can you create this PR? https://github.com/b0nete/charts/compare/b0nete:patch-1...nerdeveloper:patch-1?expand=1 |
@fede-r1c0 will @b0nete be handling this PR and working on it? if not, lets close it and move the conversation to #42. It will be easier to work on if @b0nete is unavailable. |
Sorry @cbuto, i didn't have notifications enabled i and couldn't see it in time. |
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.
no problem @b0nete! Just a few minor things to address.
You'll also need to resolve the merge conflicts.
thanks!
Co-authored-by: Casey Buto <cbuto22@gmail.com>
Co-authored-by: Casey Buto <cbuto22@gmail.com>
Co-authored-by: Casey Buto <cbuto22@gmail.com>
Co-authored-by: Casey Buto <cbuto22@gmail.com>
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.
looks good! @nerdeveloper @scbizu want to give it a look over?
lgtm! I'm guessing the docs will be updated automatically, right? @cbuto |
good point, the docs are a manual process right now. @b0nete can you add |
@fede-r1c0 that looks awesome! thanks for taking the time to update those. |
Use pathType as Prefix to work with Application Load Balancer.
closes #36