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

Fixing logic to generate ExternalHost in genericapiserver #26796

Merged
merged 2 commits into from
Jun 4, 2016

Conversation

nikhiljindal
Copy link
Contributor

@ncdc pointed it out (https://kubernetes.slack.com/archives/sig-api-machinery/p1464974528000139) that lines 305 and 306 dont match. We should be using ReadWritePort instead of ServiceReadWritePort in line 306.

#20626 seems to be the culprit PR.

@nikhiljindal nikhiljindal added the sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. label Jun 3, 2016
@nikhiljindal nikhiljindal added this to the v1.3 milestone Jun 3, 2016
@ncdc
Copy link
Member

ncdc commented Jun 3, 2016

This is a behavioral change between 1.2 and 1.3, right? re release notes

@ncdc
Copy link
Member

ncdc commented Jun 3, 2016

cc @kubernetes/sig-api-machinery @smarterclayton @liggitt @deads2k

@k8s-github-robot k8s-github-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. release-note-label-needed labels Jun 3, 2016
@k8s-bot
Copy link

k8s-bot commented Jun 3, 2016

GCE e2e build/test passed for commit 1b6e8f9.

@nikhiljindal nikhiljindal added release-note-action-required Denotes a PR that introduces potentially breaking changes that require user action. and removed release-note-label-needed labels Jun 3, 2016
@nikhiljindal
Copy link
Contributor Author

Updated the release note label

@@ -303,7 +303,7 @@ func setDefaults(c *Config) {
if len(c.ExternalHost) == 0 && c.PublicAddress != nil {
hostAndPort := c.PublicAddress.String()
if c.ReadWritePort != 0 {
Copy link
Member

Choose a reason for hiding this comment

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

This if statement will always trigger, since ReadWritePort is set on L295.

@lavalamp
Copy link
Member

lavalamp commented Jun 4, 2016

This looks correct based on the documentation for the fields.

@lavalamp lavalamp added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 4, 2016
@k8s-github-robot
Copy link

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

@k8s-bot
Copy link

k8s-bot commented Jun 4, 2016

GCE e2e build/test passed for commit 1b6e8f9.

@k8s-github-robot
Copy link

Automatic merge from submit-queue

@k8s-github-robot k8s-github-robot merged commit 74aaa14 into kubernetes:master Jun 4, 2016
@erictune
Copy link
Member

erictune commented Jul 2, 2016

@nikhiljindal Does this PR require action by the user when upgrading from 1.2.x to 1.3.0? (Think about non-developer users.) If so, please edit your first comment to have a release-note block, like in #28132. If it is just an optional feature, please change the label to just release-note. If it is not a complete feature by itself, then apply "release-note-none" label instead.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note-action-required Denotes a PR that introduces potentially breaking changes that require user action. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants