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 can not export service bug #22285

Merged
merged 1 commit into from Mar 6, 2016
Merged

fix can not export service bug #22285

merged 1 commit into from Mar 6, 2016

Conversation

adohe-zz
Copy link

@adohe-zz adohe-zz commented Mar 1, 2016

Please refer #22247 for more details.
@bgrant0607 I just add a quick&simple fix, would you mind reviewing this?

@adohe-zz adohe-zz mentioned this pull request Mar 1, 2016
Closed
@k8s-github-robot
Copy link

Labelling this PR as size/S

@k8s-github-robot k8s-github-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Mar 1, 2016
@adohe-zz
Copy link
Author

adohe-zz commented Mar 1, 2016

The Jenkins unit/integration failed for strange reason:

ERROR: Step 鈥楶ublish JUnit test result report鈥� failed: No test report files were found. Configuration error?

@k8s-bot
Copy link

k8s-bot commented Mar 1, 2016

GCE e2e build/test passed for commit 779012f78f15319ab7b06794301d441122777e15.

@bgrant0607 bgrant0607 assigned smarterclayton and unassigned lavalamp Mar 1, 2016
@bgrant0607 bgrant0607 added this to the v1.2 milestone Mar 1, 2016
@bgrant0607 bgrant0607 added priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. kind/bug Categorizes issue or PR as related to a bug. labels Mar 1, 2016
@bgrant0607
Copy link
Member 

/workspace/kubernetes/docs/api-reference is out of date. Please run hack/update-api-reference-docs.sh

smarterclayton
smarterclayton reviewed Mar 1, 2016
View reviewed changes
pkg/api/rest/rest.go
@@ -221,6 +221,7 @@ type StandardStorage interface {
GracefulDeleter
CollectionDeleter
Watcher
Exporter
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this should be required - it's desirable, but not required.

@adohe-zz
Copy link
Author

adohe-zz commented Mar 2, 2016

@smarterclayton just update this. Besides now the export output of service is:

NAME         CLUSTER-IP   EXTERNAL-IP   PORT(S)   AGE
kubernetes                <none>        443/TCP   <unknown>

I am thinking whether we should update CLUSTER-IP to <none> instead of empty.

@k8s-github-robot
Copy link

Labelling this PR as size/M

@k8s-github-robot k8s-github-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Mar 2, 2016
@k8s-bot
Copy link

k8s-bot commented Mar 2, 2016

GCE e2e build/test passed for commit e977c6424d9ee65c5909ba07652e1fdc0460a4a1.

@bgrant0607
Copy link
Member

@adohe Yes, I'd prefer to output <none> rather than leave it empty.

@k8s-github-robot
Copy link

The author of this PR is not in the whitelist for merge, can one of the admins add the 'ok-to-merge' label?

@adohe-zz
Copy link
Author

adohe-zz commented Mar 2, 2016

@bgrant0607 I think about this for a while, and I prefer to use <unknown> instead of <none>

@bgrant0607
Copy link
Member

@adohe Good idea. <unknown> SGTM.

@k8s-bot
Copy link

k8s-bot commented Mar 2, 2016

GCE e2e build/test passed for commit 49edf1b24aeaf3d68b84c5548fcd685d18501c23.

lavalamp
lavalamp reviewed Mar 2, 2016
View reviewed changes
pkg/registry/service/registry.go
}

// storage puts strong typing around storage calls
type storage struct {
rest.StandardStorage
ss rest.StandardStorage
Copy link
Member

Choose a reason for hiding this comment

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

Why did you name this?

Copy link
Member

Choose a reason for hiding this comment

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

Please leave unnamed, or if you have to name it, use a name like 'delegate' or something else more descriptive. Tiny names are good for local variable names in short functions, but not good as members in structs.

@lavalamp
Copy link
Member

lavalamp commented Mar 2, 2016

Minor comments.

@k8s-bot
Copy link

k8s-bot commented Mar 3, 2016

GCE e2e build/test passed for commit 990ae2011e47478ccae3c1b7d4c5ca0d00cfc6f2.

@bgrant0607 bgrant0607 assigned lavalamp and unassigned smarterclayton Mar 3, 2016
@k8s-github-robot
Copy link

@adohe PR needs rebase

@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 4, 2016
@lavalamp
Copy link
Member

lavalamp commented Mar 4, 2016

LGTM when rebased, thanks

@k8s-github-robot k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 5, 2016
@k8s-bot
Copy link

k8s-bot commented Mar 5, 2016

GCE e2e build/test passed for commit c61cd2e8527fd7927b9a53a167b8395f811c75ea.

@k8s-bot
Copy link

k8s-bot commented Mar 5, 2016

GCE e2e build/test passed for commit b9958000ff5776e85e23040f0a98a8b24e1bf302.

@k8s-bot
Copy link

k8s-bot commented Mar 5, 2016

GCE e2e build/test passed for commit 5fdfc4b.

@bgrant0607 bgrant0607 added lgtm "Looks good to me", indicates that a PR is ready to be merged. cherrypick-candidate labels Mar 6, 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 Mar 6, 2016

GCE e2e build/test passed for commit 5fdfc4b.

@k8s-github-robot
Copy link

Automatic merge from submit-queue

k8s-github-robot pushed a commit that referenced this pull request Mar 6, 2016
Merge pull request #22285 from AdoHe/fix_service_export
bc29585 
Auto commit by PR queue bot
@k8s-github-robot k8s-github-robot merged commit bc29585 into kubernetes:master Mar 6, 2016
@adohe-zz adohe-zz deleted the fix_service_export branch March 7, 2016 02:26
@ethernetdan ethernetdan mentioned this pull request Mar 10, 2016
Closed
@adohe-zz adohe-zz mentioned this pull request Apr 27, 2016
Closed
3 tasks
0xmichalis
0xmichalis reviewed Apr 27, 2016
View reviewed changes
pkg/registry/service/strategy.go
@@ -90,7 +90,7 @@ func (svcStrategy) Export(obj runtime.Object, exact bool) error {
return nil
}
if t.Spec.ClusterIP != api.ClusterIPNone {
t.Spec.ClusterIP = ""
t.Spec.ClusterIP = "<unknown>"
Copy link
Contributor

Choose a reason for hiding this comment

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

Why this change?

Copy link
Contributor

Choose a reason for hiding this comment

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

This prevents the exported resource from being created on another cluster, which defeats the purpose of export.

Copy link
Member

Choose a reason for hiding this comment

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

facing issue - #47976, #28548

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mkumatag mkumatag mkumatag left review comments

Assignees

@lavalamp lavalamp

Labels
kind/bug Categorizes issue or PR as related to a bug. 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. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
v1.2
Development

Successfully merging this pull request may close these issues.

None yet