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

Detect if replicas have been defaulted in rolling-update and use old replica count #10234

Merged
merged 4 commits into from
Jun 26, 2015
Merged

Detect if replicas have been defaulted in rolling-update and use old replica count #10234

merged 4 commits into from
Jun 26, 2015

Conversation

mikedanese
Copy link
Member

@bgrant0607 bgrant0607 added this to the v1.0 milestone Jun 23, 2015
@bgrant0607
Copy link
Member

cc @jlowdermilk @ironcladlou

@bgrant0607
Copy link
Member

Thanks. I'll try to take a look as soon as I can, but not assigning to myself since I'm at Dockercon today.

@smarterclayton might also want to take a look.

Vaguely related to #7111, which (among other things) discusses decoupling unmarshaling from conversion.

@smarterclayton
Copy link
Contributor

I'll try although I'm at Red Hat Summit :)

On Jun 23, 2015, at 1:21 PM, Brian Grant notifications@github.com wrote:

Thanks. I'll try to take a look as soon as I can, but not assigning to myself since I'm at Dockercon today.

@smarterclayton might also want to take a look.

Vaguely related to #7111, which (among other things) discusses decoupling unmarshaling from conversion.

—
Reply to this email directly or view it on GitHub.

@mikedanese mikedanese force-pushed the rolling-update-weird branch from 5ebfacd to 0f9aece Compare June 23, 2015 17:41
j3ffml
j3ffml reviewed Jun 24, 2015
View reviewed changes
pkg/kubectl/resource/mapper.go
Namespace: namespace,
Name: name,
var versionedInfo *VersionedInfo
versionedObj, _, _, err := api.Scheme.Raw().DecodeToVersionedObject(data)
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm pretty sure you don't need to add a VersionedInfo object to Info. The api version is already available via Info.Mapping which is a RESTMapping.

@mikedanese mikedanese force-pushed the rolling-update-weird branch from 0f9aece to 9a80f24 Compare June 24, 2015 02:44
j3ffml
j3ffml reviewed Jun 24, 2015
View reviewed changes
pkg/api/v1beta3/types.go
@@ -990,7 +990,7 @@ type PodTemplateList struct {
// ReplicationControllerSpec is the specification of a replication controller.
type ReplicationControllerSpec struct {
// Replicas is the number of desired replicas.
Copy link
Contributor

Choose a reason for hiding this comment

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

Would be helpful for comment to mention that it's a pointer to distinguish between explicitly zero and unspecified.

@mikedanese mikedanese force-pushed the rolling-update-weird branch 2 times, most recently from 99300ad to f9b520d Compare June 24, 2015 18:16
j3ffml
j3ffml reviewed Jun 24, 2015
View reviewed changes
pkg/kubectl/resource/visitor.go
@@ -66,6 +66,10 @@ type Info struct {
Namespace string
Name string

// Optional, this is the provided object in a versioned type before defaulting
// and conversions into it's corresponding internal type. This is useful for
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: s/it's/its

@j3ffml
Copy link
Contributor

j3ffml commented Jun 24, 2015

LGTM, but @smarterclayton and/or @bgrant0607 should take a look.

@mikedanese mikedanese force-pushed the rolling-update-weird branch from f9b520d to df2892a Compare June 24, 2015 21:10
@k8s-bot
Copy link

k8s-bot commented Jun 24, 2015

GCE e2e build/test passed for commit df2892a38ad77c8507bd2a38ac488ba23f658c13.

@bgrant0607 bgrant0607 assigned bgrant0607 and unassigned j3ffml Jun 25, 2015
@mikedanese mikedanese force-pushed the rolling-update-weird branch from df2892a to dd07df0 Compare June 25, 2015 19:29
@bgrant0607 bgrant0607 added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 26, 2015
@bgrant0607
Copy link
Member

LGTM

@bgrant0607
Copy link
Member

ok to merge

@bgrant0607 bgrant0607 mentioned this pull request Jun 26, 2015
Merged
@roberthbailey
Copy link
Contributor

@mikedanese want to kick the jenkins bot to test this?

@mikedanese
Copy link
Member Author

@k8s-bot ok to test

@k8s-bot
Copy link

k8s-bot commented Jun 26, 2015

GCE e2e build/test passed for commit dd07df0.

@mikedanese
Copy link
Member Author

@roberthbailey k8s-bot says LGTM

@roberthbailey
Copy link
Contributor

Thanks. gce-e2e just went red so I'll merge once it is happy again.

roberthbailey added a commit that referenced this pull request Jun 26, 2015
@roberthbailey
Merge pull request #10234 from mikedanese/rolling-update-weird
f9db614 
Detect if replicas have been defaulted in rolling-update and use old replica count
@roberthbailey roberthbailey merged commit f9db614 into kubernetes:master Jun 26, 2015
@mikedanese mikedanese deleted the rolling-update-weird branch June 26, 2015 20:16
@bgrant0607 bgrant0607 mentioned this pull request Jul 14, 2015
Merged
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
lgtm "Looks good to me", indicates that a PR is ready to be merged.
Projects
None yet
Milestone
v1.0
Development

Successfully merging this pull request may close these issues.
7 participants
@mikedanese @bgrant0607 @smarterclayton @j3ffml @k8s-bot @roberthbailey @googlebot