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

Fix unintended change of Service.spec.ports[].nodePort during kubectl apply #24180

Merged
merged 1 commit into from
Apr 20, 2016
Merged

Fix unintended change of Service.spec.ports[].nodePort during kubectl apply #24180

merged 1 commit into from
Apr 20, 2016

Conversation

adohe-zz
Copy link

@adohe-zz adohe-zz commented Apr 13, 2016 •
edited by thockin
Loading

Please refer #23551 for more detail. @bgrant0607 I think this simple fix should be ok to reuse nodePort. @thockin ptal.

Release note: Fix unintended change of Service.spec.ports[].nodePort during kubectl apply.

@k8s-github-robot k8s-github-robot added kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API size/S Denotes a PR that changes 10-29 lines, ignoring generated files. release-note-label-needed labels Apr 13, 2016
@k8s-bot
Copy link

k8s-bot commented Apr 13, 2016

GCE e2e build/test passed for commit 84e975dac93f8eb43bbeb8bcee57b548a3c06749.

@bgrant0607 bgrant0607 assigned bgrant0607 and unassigned spxtr Apr 13, 2016
@bgrant0607
Copy link
Member

Thanks @adohe. LGTM.

@bgrant0607 bgrant0607 added priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Apr 13, 2016
@k8s-github-robot k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 13, 2016
@thockin thockin added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-label-needed labels Apr 16, 2016
@thockin
Copy link
Member

thockin commented Apr 16, 2016

I added a release note and am re-LGTMing

@thockin thockin added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 16, 2016
@thockin
Copy link
Member

thockin commented Apr 16, 2016

So no need to make it a pointer after all?

@k8s-bot
Copy link

k8s-bot commented Apr 16, 2016

GCE e2e build/test failed for commit 84e975dac93f8eb43bbeb8bcee57b548a3c06749.

Please reference the list of currently known flakes when examining this failure. If you request a re-test, you must reference the issue describing the flake.

@k8s-bot
Copy link

k8s-bot commented Apr 16, 2016

GCE e2e build/test failed for commit 84e975dac93f8eb43bbeb8bcee57b548a3c06749.

Please reference the list of currently known flakes when examining this failure. If you request a re-test, you must reference the issue describing the flake.

@adohe-zz
Copy link
Author

@thockin yes, no need to make it pointer.

@thockin thockin added this to the v1.2 milestone Apr 17, 2016
@thockin thockin mentioned this pull request Apr 18, 2016
Closed
@thockin thockin changed the title fix reuse nodePort issue Fix unintended change of Service.spec.ports[].nodePort during kubectl apply Apr 18, 2016
@bgrant0607
Copy link
Member

@adohe Tests failed. Maybe needs rebase?

../src/k8s.io/kubernetes/test/e2e/kubectl.go:565: undefined: runKubectlOrDie

roberthbailey
roberthbailey reviewed Apr 19, 2016
View reviewed changes
pkg/api/v1/types.go
@@ -1780,7 +1780,7 @@ type LoadBalancerIngress struct {
type ServiceSpec struct {
// The list of ports that are exposed by this service.
// More info: http://releases.k8s.io/HEAD/docs/user-guide/services.md#virtual-ips-and-service-proxies
Ports []ServicePort `json:"ports"`
Ports []ServicePort `json:"ports" patchStrategy:"merge" patchMergeKey:"port"`
Copy link
Contributor

Choose a reason for hiding this comment

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

This is fully backwards compatible, right?

Copy link
Member

Choose a reason for hiding this comment

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

@roberthbailey It will change patch and apply behavior for this resource, though the current behavior is unusable.

Copy link
Member

Choose a reason for hiding this comment

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

If someone did manage to workaround this problem and was using strategic merge patch, their patches should still work.

Copy link
Contributor

Choose a reason for hiding this comment

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

thanks.

@k8s-bot
Copy link

k8s-bot commented Apr 20, 2016

GCE e2e build/test failed for commit fcdf2161353631121005a26f2d57d97c5aa5c127.

Please reference the list of currently known flakes when examining this failure. If you request a re-test, you must reference the issue describing the flake.

@k8s-github-robot k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 20, 2016
@zmerlynn
Copy link
Member

This PR is in an odd state. If you want to get it in as a cherrypick for 1.2.3, please get it merged soon and poke me for the cherrypick-approved label.

@adohe-zz
Copy link
Author

I am fixing this.

@k8s-bot
Copy link

k8s-bot commented Apr 20, 2016

GCE e2e build/test passed for commit 16960d3.

@adohe-zz
Copy link
Author

@bgrant0607 ptal.

@bgrant0607
Copy link
Member

LGTM, thanks!

@bgrant0607 bgrant0607 added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 20, 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 Apr 20, 2016

GCE e2e build/test passed for commit 16960d3.

@k8s-github-robot
Copy link

Automatic merge from submit-queue

@k8s-github-robot k8s-github-robot merged commit 3753e2b into kubernetes:master Apr 20, 2016
@zmerlynn zmerlynn added the cherry-pick-approved Indicates a cherry-pick PR into a release branch has been approved by the release branch manager. label Apr 20, 2016
@zmerlynn
Copy link
Member

@adohe: Please create a cherrypick PR for this ASAP. Thanks!

@thockin
Copy link
Member

thockin commented Apr 20, 2016

or tell us and we can do it, if you don't have time :)

On Wed, Apr 20, 2016 at 10:45 AM, Zach Loafman notifications@github.com
wrote:

@adohe https://github.com/AdoHe: Please create a cherrypick PR for this
ASAP. Thanks!

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#24180 (comment)

@zmerlynn
Copy link
Member

I tried quickly to create one and there's a conflict:

+++ Conflicts detected:

UU pkg/api/v1/types.go

That's as far as Zach-bot goes.

@zmerlynn
Copy link
Member

zmerlynn commented Apr 20, 2016 •
edited
Loading

This PR is unlikely to make it in to 1.2.3. Please tell me immediately if I need to block 1.2.3 on it.

@adohe-zz
Copy link
Author

@zmerlynn what can I do for this? I just go back from my holiday. And now have more time to do this.

@zmerlynn
Copy link
Member

Please follow the procedure at https://github.com/eBay/Kubernetes/blob/master/docs/devel/cherry-picks.md to create a cherry pick against upstream/release-1.2. Thanks!

This was referenced Apr 21, 2016
Closed
Merged
zmerlynn added a commit that referenced this pull request Apr 21, 2016
@zmerlynn
Merge pull request #24581 from AdoHe/automated-cherry-pick-of-#24180-…
7686572 
…upstream-release-1.2

Automated cherry pick of #24180
@k8s-cherrypick-bot
Copy link

Commit found in the "release-1.2" branch appears to be this PR. Removing the "cherrypick-candidate" label. If this is an error find help to get your PR picked.

@mafraba mafraba mentioned this pull request Apr 29, 2016
Open
@adohe-zz adohe-zz deleted the reuse_node_port branch May 7, 2016 13:48
shyamjvs pushed a commit to shyamjvs/kubernetes that referenced this pull request Dec 1, 2016
@zmerlynn
Merge pull request kubernetes#24581 from AdoHe/automated-cherry-pick-…
254e3eb 
…of-#24180-upstream-release-1.2

Automated cherry pick of kubernetes#24180
@mengqiy mengqiy mentioned this pull request Dec 3, 2016
Merged
shouhong pushed a commit to shouhong/kubernetes that referenced this pull request Feb 14, 2017
@zmerlynn
Merge pull request kubernetes#24581 from AdoHe/automated-cherry-pick-…
2a19cfd 
…of-#24180-upstream-release-1.2

Automated cherry pick of kubernetes#24180
openshift-publish-robot pushed a commit to openshift/kubernetes that referenced this pull request Nov 20, 2019
@k8s-publishing-bot
Merge pull request kubernetes#24180 from deads2k/controller-ca
d1d15cd 
UPSTREAM: 82705: use controller to publish cluster authentication info

Origin-commit: bd8621462110dc03396cc304b3579281e184840a
openshift-publish-robot pushed a commit to openshift/kubernetes that referenced this pull request Dec 9, 2019
@k8s-publishing-bot
Merge pull request kubernetes#24180 from deads2k/controller-ca
629d799 
UPSTREAM: 82705: use controller to publish cluster authentication info

Origin-commit: bd8621462110dc03396cc304b3579281e184840a
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@bgrant0607 bgrant0607

Labels
cherry-pick-approved Indicates a cherry-pick PR into a release branch has been approved by the release branch manager. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Milestone
v1.2
Development

Successfully merging this pull request may close these issues.