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.

Sign up for GitHub

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

Jump to bottom

r/service: Make spec.port.target_port optional #69

Merged
merged 1 commit into from
Oct 3, 2017
Merged

r/service: Make spec.port.target_port optional #69

merged 1 commit into from
Oct 3, 2017

Conversation

radeksimko
Copy link
Member

Fixes #67

@radeksimko radeksimko added the bug label Sep 1, 2017
@radeksimko radeksimko mentioned this pull request Sep 1, 2017
Closed
catsby
catsby reviewed Sep 7, 2017
View reviewed changes
Copy link
Member

@catsby catsby left a comment

Choose a reason for hiding this comment

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

What is the behavior if someone removes target_port from their configuration? Is there a Default we could use?

@radeksimko
Copy link
Member Author

As mentioned in the linked issue:

By default the targetPort will be set to the same value as the port field.

so we can't set a static Default hence I made it Computed.

catsby
catsby approved these changes Oct 2, 2017
View reviewed changes
Copy link
Member

@catsby catsby left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for the follow up

@radeksimko radeksimko merged commit 49a1b59 into master Oct 3, 2017
@radeksimko radeksimko deleted the b-svc-optional-target-port branch October 3, 2017 06:48
@ghost ghost locked and limited conversation to collaborators Apr 21, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@catsby catsby catsby approved these changes

Assignees
No one assigned
Labels
bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Service errors when optional param is not supplied and fails to set annotation
2 participants
@radeksimko @catsby