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

Allow replica set to be exposed as a service and add kubectl command tests for replica sets. #20928

Merged
merged 1 commit into from
Feb 11, 2016
Merged

Allow replica set to be exposed as a service and add kubectl command tests for replica sets. #20928

merged 1 commit into from
Feb 11, 2016
+218 −21

Conversation

madhusudancs
Copy link
Contributor

Only the last commit needs review.

cc @bgrant0607

@madhusudancs madhusudancs mentioned this pull request Feb 9, 2016
Merged
@janetkuo
Copy link
Member

janetkuo commented Feb 9, 2016

Please include replicasets in pkg/kubectl/cmd/expose.go long message and example too.

@k8s-github-robot
Copy link

Labelling this PR as size/XXL

@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/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Feb 9, 2016
@bgrant0607
Copy link
Member

cc @Kargakis

@k8s-bot
Copy link

k8s-bot commented Feb 9, 2016

GCE e2e test build/test passed for commit 0408470e5d4667c83a01add1c831a9fd7c41aeda.

@k8s-bot
Copy link

k8s-bot commented Feb 9, 2016

GCE e2e test build/test passed for commit b306d6fe6bfd76189eb476a6e5fedfcf12300687.

@madhusudancs
Copy link
Contributor Author

Please include replicasets in pkg/kubectl/cmd/expose.go long message and example too.

@janetkuo Done.

@madhusudancs madhusudancs force-pushed the replicaset-expose branch 2 times, most recently from 57c0e73 to eaa7ac8 Compare February 9, 2016 23:03
@k8s-bot
Copy link

k8s-bot commented Feb 9, 2016

GCE e2e test build/test passed for commit 57c0e730d00f4c813524fc0814d3060682978547.

@k8s-bot
Copy link

k8s-bot commented Feb 9, 2016

GCE e2e test build/test passed for commit eaa7ac8.

@bgrant0607
Copy link
Member

Note for others: I'm commenting on the last commit.

@bgrant0607
Copy link
Member

@janetkuo Please move the deployment example from /examples/extensions to /docs/user-guide

@bgrant0607 bgrant0607 mentioned this pull request Feb 10, 2016
Merged
@janetkuo janetkuo mentioned this pull request Feb 10, 2016
Merged
@janetkuo
Copy link
Member

#3024

@0xmichalis
Copy link
Contributor

You also need to handle rs in PortsForObject in factory.go for expose.
Can you open issues for rs scaler, rs reaper, and rs autoscaler?

@madhusudancs madhusudancs added this to the v1.2 milestone Feb 10, 2016
@madhusudancs
Copy link
Contributor Author

Is kubectl scale implemented using the scale subresource, or does it just update the resource?

@bgrant0607 It threw me off a bit as well (after reading your comment). Because I remembered commenting out Deployment scaler. I had to go look at the code again to remind myself. We just update the resource -

kubernetes/pkg/kubectl/scale.go

Line 217 in e7a9f30

rs.Spec.Replicas = int(newSize)

@0xmichalis
Copy link
Contributor

Once the scale subresource is considered ready, I would vote for changing kubectl scale to use the scale endpoints (what already happens with deployments)

@k8s-bot
Copy link

k8s-bot commented Feb 10, 2016

GCE e2e build/test failed for commit c9c26b4b08085392e18852f4f52fcd0f784ab762.

@madhusudancs
Copy link
Contributor Author

You also need to handle rs in PortsForObject in factory.go for expose.

Done.

Can you open issues for rs scaler, rs reaper, and rs autoscaler?

Scaler and Reaper are already implemented. Filed #20986 for kubectl autoscale.

@k8s-github-robot
Copy link

Labelling this PR as size/L

@0xmichalis
Copy link
Contributor

LGTM

@k8s-bot
Copy link

k8s-bot commented Feb 10, 2016

GCE e2e test build/test passed for commit 99e592ff1602943702b9c58cdff8b74deb40077b.

bgrant0607
bgrant0607 reviewed Feb 10, 2016
View reviewed changes
pkg/kubectl/history.go
@@ -67,22 +67,22 @@ func (h *DeploymentHistoryViewer) History(namespace, name string) (HistoryInfo,
if err != nil {
return historyInfo, fmt.Errorf("failed to retrieve deployment %s: %v", name, err)
}
_, allOldRCs, err := deploymentutil.GetOldReplicaSets(*deployment, h.c)
_, allOldRSs, err := deploymentutil.GetOldReplicaSets(*deployment, h.c)
Copy link
Member

Choose a reason for hiding this comment

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

This fix is unrelated to this PR, but ok.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bgrant0607 This was an artifact of the weird commits ordering, which itself was due to an incorrect rebase. Fixed the rebase and squashed the commits. This has disappeared now.

@bgrant0607
Copy link
Member

@madhusudancs If the yaml files are just for the test and not examples for users, then we probably should find a better place for them later.

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

The 3 commits appear in a weird order. Please rebase and squash.

@bgrant0607
Copy link
Member

Don't change it now, but hack/testdata is probably the right place for test-cmd.sh inputs.

@madhusudancs madhusudancs removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 11, 2016
@madhusudancs
Copy link
Contributor Author

This is next in the queue to be merged and as @bgrant0607 mentioned the commits are in a strange order. I am on a bus and don't know when SSH just drops. So removing the LGTM label for now. Will add it back once the PR is rebased and the commits are squashed.

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

PR needs rebase

@k8s-github-robot k8s-github-robot removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API labels Feb 11, 2016
@k8s-bot
Copy link

k8s-bot commented Feb 11, 2016

GCE e2e test build/test passed for commit 293793c.

@madhusudancs madhusudancs added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 11, 2016
@madhusudancs
Copy link
Contributor Author

Everything looks Ok, adding LGTM back.

@k8s-github-robot
Copy link

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

@bgrant0607
Copy link
Member

Thanks. LGTM

@k8s-bot
Copy link

k8s-bot commented Feb 11, 2016

GCE e2e build/test failed for commit 293793c.

@madhusudancs
Copy link
Contributor Author

@k8s-bot test this issue: #21045

@k8s-bot
Copy link

k8s-bot commented Feb 11, 2016

GCE e2e test build/test passed for commit 293793c.

@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 Feb 11, 2016

GCE e2e test build/test passed for commit 293793c.

@k8s-github-robot
Copy link

Automatic merge from submit-queue

k8s-github-robot pushed a commit that referenced this pull request Feb 11, 2016
Merge pull request #20928 from madhusudancs/replicaset-expose
3208665 
Auto commit by PR queue bot
@k8s-github-robot k8s-github-robot merged commit 3208665 into kubernetes:master Feb 11, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@janetkuo janetkuo

Labels
area/kubectl lgtm "Looks good to me", indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
v1.2
Development

Successfully merging this pull request may close these issues.
8 participants
@madhusudancs @janetkuo @k8s-github-robot @bgrant0607 @k8s-bot @0xmichalis @k8s-teamcity-mesosphere @googlebot