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

Introducing ReplicaSet, a new incarnation of Replication Controller. #19042

Merged
merged 1 commit into from
Jan 21, 2016
Merged

Introducing ReplicaSet, a new incarnation of Replication Controller. #19042

merged 1 commit into from
Jan 21, 2016

Conversation

madhusudancs
Copy link
Contributor

This commit adds the API types, conversion, validation and defaulting code.

Ref #3024

@madhusudancs
Copy link
Contributor Author

cc @davidopp @nikhiljindal @mikedanese

@k8s-bot
Copy link

k8s-bot commented Dec 23, 2015

GCE e2e build/test failed for commit 3f4f4e126ede487252da17b0bb649be4c892f548.

@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 kind/new-api size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Dec 23, 2015
@mqliang mqliang mentioned this pull request Dec 27, 2015
Closed
nikhiljindal
nikhiljindal reviewed Dec 28, 2015
View reviewed changes
pkg/apis/extensions/types.go
Replicas int `json:"replicas"`

// ObservedGeneration is the most recent generation observed by the controller.
ObservedGeneration int64 `json:"observedGeneration,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

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

As per api-conventions.md, internal types should be int. External type can be int64.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

api-convensions.md says "Internal types may use (u)int." here - https://github.com/kubernetes/kubernetes/blob/master/docs/devel/api-conventions.md#primitive-types. Am I missing something here?

Also, I think ObservedGeneration should use int64 for both public and internal types to preserve precision.

Copy link
Member

Choose a reason for hiding this comment

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

@nikhiljindal What in api-conventions.md suggests this can't be int64? We'd lose precision if it were only int.

@madhusudancs Please keep this int64.

Copy link
Contributor

Choose a reason for hiding this comment

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

ok sorry. looks like I mis-interpreted it.
It says "All public integer fields MUST use the Go (u)int32 or Go (u)int64 types, not (u)int (which is ambiguous depending on target platform). Internal types may use (u)int."

@nikhiljindal
Copy link
Contributor

Thanks for separating your code and generated code into separate commits (there is a diff of 18k in the auto-generated code!!).
Please add tests in validation_test.go and defaults_test.go.
Also you need to update your generated code for travis to pass.

@googlebot
Copy link

We found a Contributor License Agreement for you (the sender of this pull request) and all commit authors, but as best as we can tell these commits were authored by someone else. If that's the case, please add them to this pull request and have them confirm that they're okay with these commits being contributed to Google. If we're mistaken and you did author these commits, just reply here to confirm.

@googlebot
Copy link

CLAs look good, thanks!

@madhusudancs
Copy link
Contributor Author

@nikhiljindal No problem. Thanks for the quick review. Have addressed the comments. PTAL.

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

k8s-bot commented Dec 29, 2015

GCE e2e build/test failed for commit 5a0ae658c441777c25a239f71964d7362fa25b98.

@k8s-bot
Copy link

k8s-bot commented Dec 29, 2015

GCE e2e test build/test passed for commit 09b17a9586cdb196a098b1764d5db16fbf7e7f0b.

@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 29, 2015
bgrant0607
bgrant0607 reviewed Jan 6, 2016
View reviewed changes
pkg/apis/extensions/types.go
// TemplateRef is a reference to an object that describes the pod that will be created if
// insufficient replicas are detected. This reference is ignored if a Template is set.
// Must be set before converting to a versioned API object
//TemplateRef *ObjectReference `json:"templateRef,omitempty"`
Copy link
Member

Choose a reason for hiding this comment

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

Please just remove the TemplateRef comment for now, here and in v1/types.go.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@bgrant0607 I have also changed the ReplicaSetSpec doc/comment above to

// ReplicaSetSpec is the specification of a ReplicaSet.
// As the internal representation of a ReplicaSet, it must have
// a Template set.

Is that accurate?

@k8s-bot
Copy link

k8s-bot commented Jan 19, 2016

GCE e2e test build/test passed for commit d0e70edbbfce1954af18f25e06d97cff5ab0291e.

@k8s-bot
Copy link

k8s-bot commented Jan 19, 2016

GCE e2e test build/test passed for commit 7e11821b93008821cbcf56a94a5101d9d87bc86f.

@nikhiljindal
Copy link
Contributor

Are the first 2 commits intended?
They dont seem to be related to this PR.

@madhusudancs
Copy link
Contributor Author

Are the first 2 commits intended?
They dont seem to be related to this PR.

@nikhiljindal Those two commits being here is intentional, but it shouldn't be merged with this PR though. I am keeping that in mind, but it is better if I also write it down here.

Those two commits, along with the commits in this PR are required for the future PRs which build on these changes. There is neither an easy way to define base PRs for a given PR nor is it easy to define two parent PRs for a given PR unless I create an actual merge commit. I am afraid to create a merge commits because it is hard to manage with rebases.

So I am ordering the PRs one after the other, with PR #19802 being the first PR followed by this PR. If you think this PR is good to go, I will reorder the PRs, so that those commits disappear from this PR. Let me know.

@madhusudancs
Copy link
Contributor Author

@nikhiljindal Reordered the PRs, those two commits should have disappeared now. PTAL.

@k8s-github-robot
Copy link

Labelling this PR as size/XL

@k8s-github-robot k8s-github-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jan 19, 2016
@k8s-bot
Copy link

k8s-bot commented Jan 19, 2016

GCE e2e test build/test passed for commit f6f658a69c0edd49389413431a7434c4497f6d28.

@k8s-bot
Copy link

k8s-bot commented Jan 20, 2016

GCE e2e test build/test passed for commit 951fa9dc246ecc79ded61ec768dbb5045c736421.

@nikhiljindal
Copy link
Contributor

LGTM, thanks!

@nikhiljindal nikhiljindal added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 20, 2016
@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 21, 2016
@madhusudancs
Introducing ReplicaSet, a new incarnation of Replication Controller.
572abe1 
This commit adds the API types, conversion, validation and defaulting code.
@madhusudancs madhusudancs added lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Jan 21, 2016
@k8s-github-robot k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 21, 2016
@k8s-bot
Copy link

k8s-bot commented Jan 21, 2016

GCE e2e test build/test passed for commit 572abe1.

@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 Jan 21, 2016

GCE e2e test build/test passed for commit 572abe1.

@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 Jan 21, 2016

GCE e2e test build/test passed for commit 572abe1.

@k8s-github-robot
Copy link

Automatic merge from submit-queue

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

@nikhiljindal nikhiljindal

Labels
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. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@madhusudancs @k8s-bot @k8s-github-robot @nikhiljindal @googlebot @bgrant0607 @k8s-teamcity-mesosphere