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

Refactor IntOrString into a new pkg #14469

Merged
merged 1 commit into from
Nov 17, 2015
Merged

Refactor IntOrString into a new pkg #14469

merged 1 commit into from
Nov 17, 2015

Conversation

thockin
Copy link
Member

@thockin thockin commented Sep 24, 2015

pkg/util/intstr is a cleaner encapsulation for this type and supporting
functions. No behavioral changes.

This has annoyed me for a long time, and one too many code reviews that touched
it finally put me over the edge.

@k8s-bot
Copy link

k8s-bot commented Sep 24, 2015

GCE e2e build/test failed for commit 157846dd3f799854ea07833d46008594beea27cd.

@k8s-github-robot k8s-github-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Sep 24, 2015
@k8s-github-robot
Copy link

Labelling this PR as size/XXL

@thockin
Copy link
Member Author

thockin commented Sep 24, 2015

@k8s-bot test this please

@k8s-bot
Copy link

k8s-bot commented Sep 24, 2015

GCE e2e build/test passed for commit 157846dd3f799854ea07833d46008594beea27cd.

@derekwaynecarr
Copy link
Member

This reads a lot better ;-)

derekwaynecarr
derekwaynecarr reviewed Sep 24, 2015
View reviewed changes
pkg/util/intstr/intstr_test.go
@@ -0,0 +1,111 @@
/*
Copyright 2014 The Kubernetes Authors All rights reserved.
Copy link
Member

Choose a reason for hiding this comment

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

2015

Copy link
Member Author

Choose a reason for hiding this comment

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

The original files were 2014, we're not supposed to update copyrights

@derekwaynecarr
Copy link
Member

minor nit on copyright, otherwise LGTM

@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 24, 2015
@derekwaynecarr
Copy link
Member

@thockin - sorry for the confusion, i thought new files got new header. needs a rebase.

@thockin thockin removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 25, 2015
@thockin
Copy link
Member Author

thockin commented Sep 25, 2015

rebased, tests re-running

@k8s-bot
Copy link

k8s-bot commented Sep 25, 2015

GCE e2e build/test passed for commit 5f0282ca07696bf4a4d7c809e230a5580df1a51c.

@thockin
Copy link
Member Author

thockin commented Sep 25, 2015

50/50 on shippable

@derekwaynecarr
Copy link
Member

Ships me has been flaky

On Friday, September 25, 2015, Tim Hockin notifications@github.com wrote:

50/50 on shippable

—
Reply to this email directly or view it on GitHub
#14469 (comment)
.

@derekwaynecarr derekwaynecarr added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 26, 2015
@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 Sep 29, 2015

Unit, integration and GCE e2e build/test failed for commit 5f0282ca07696bf4a4d7c809e230a5580df1a51c.

@brendandburns
Copy link
Contributor

@k8s-bot test this please

@k8s-bot
Copy link

k8s-bot commented Sep 29, 2015

Unit, integration and GCE e2e test build/test passed for commit 5f0282ca07696bf4a4d7c809e230a5580df1a51c.

@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 29, 2015
@thockin thockin removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 1, 2015
@k8s-bot
Copy link

k8s-bot commented Oct 1, 2015

Unit, integration and GCE e2e test build/test passed for commit ba4d75154b00b92ec23d53e55af362568da081c3.

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

@k8s-bot test this

@derekwaynecarr
Copy link
Member

Looks like a compile error:

[20:12:08][Step 4/4] _output/local/go/src/k8s.io/kubernetes/pkg/util/util.go:36:2: cannot find package "k8s.io/kubernetes/pkg/util/intstr" in any of:
[20:12:08][Step 4/4]    /usr/src/go/src/k8s.io/kubernetes/pkg/util/intstr (from $GOROOT)
[20:12:08][Step 4/4]    /go/src/github.com/GoogleCloudPlatform/kubernetes/_output/local/go/src/k8s.io/kubernetes/pkg/util/intstr (from $GOPATH)
[20:12:08][Step 4/4]    /go/src/github.com/GoogleCloudPlatform/kubernetes/Godeps/_workspace/src/k8s.io/kubernetes/pkg/util/intstr
[20:12:08][Step 4/4] !!! Error in /go/src/github.com/GoogleCloudPlatform/kubernetes/hack/lib/golang.sh:392
[20:12:08][Step 4/4]   'go install "${goflags[@]:+${goflags[@]}}" -ldflags "${goldflags}" "${nonstatics[@]:+${nonstatics[@]}}"' exited with status 1
[20:12:08][Step 4/4] Call stack:
[20:12:08][Step 4/4]   1: /go/src/github.com/GoogleCloudPlatform/kubernetes/hack/lib/golang.sh:392 kube::golang::build_binaries_for_platform(...)
[20:12:08][Step 4/4]   2: /go/src/github.com/GoogleCloudPlatform/kubernetes/hack/lib/golang.sh:527 kube::golang::build_binaries(...)
[20:12:08][Step 4/4]   3: hack/build-go.sh:26 main(...)
[20:12:08][Step 4/4] Exiting with status 1
[20:12:08][Step 4/4] !!! Error in /go/src/github.com/GoogleCloudPlatform/kubernetes/hack/lib/golang.sh:448

@k8s-bot
Copy link

k8s-bot commented Nov 11, 2015

GCE e2e build/test failed for commit 2cc4b502de6e372c6438d86532ea12375f7ff4a7.

@thockin
Copy link
Member Author

thockin commented Nov 12, 2015

@k8s-bot test this please

@k8s-bot unit test this please

@k8s-bot
Copy link

k8s-bot commented Nov 12, 2015

GCE e2e build/test failed for commit 2cc4b502de6e372c6438d86532ea12375f7ff4a7.

@thockin
Copy link
Member Author

thockin commented Nov 12, 2015

@ixdy I need an assist. This is failing because it can't find a package that was added in this PR.

13:20:52 +++ [1112 21:20:52] Building go targets for linux/amd64:
13:20:52     cmd/kube-proxy
13:20:52     cmd/kube-apiserver
13:20:52     cmd/kube-controller-manager
13:20:52     cmd/kubelet
13:20:52     cmd/kubemark
13:20:52     cmd/hyperkube
13:20:52     cmd/linkcheck
13:20:52     plugin/cmd/kube-scheduler
13:20:52 _output/dockerized/go/src/k8s.io/kubernetes/pkg/util/util.go:36:2: cannot find package "k8s.io/kubernetes/pkg/util/intstr" in any of:
13:20:52    /usr/src/go/src/k8s.io/kubernetes/pkg/util/intstr (from $GOROOT)
13:20:52    /go/src/k8s.io/kubernetes/_output/dockerized/go/src/k8s.io/kubernetes/pkg/util/intstr (from $GOPATH)
13:20:52    /go/src/k8s.io/kubernetes/Godeps/_workspace/src/k8s.io/kubernetes/pkg/util/intstr
13:20:52 !!! Error in /go/src/k8s.io/kubernetes/hack/lib/golang.sh:392
13:20:52   'go install "${goflags[@]:+${goflags[@]}}" -ldflags "${goldflags}" "${nonstatics[@]:+${nonstatics[@]}}"' exited with status 1

This seems like a failure of test infra - not the PR. Help?

@thockin
Copy link
Member Author

thockin commented Nov 14, 2015

NM, I screwed up. Always blame the tools first :) trying again.

@k8s-bot
Copy link

k8s-bot commented Nov 14, 2015

GCE e2e build/test failed for commit 3d11805b7fad787608392464ccb8eb6d875ccad0.

@k8s-bot
Copy link

k8s-bot commented Nov 14, 2015

GCE e2e test build/test passed for commit ac7621169ffb329b0405dac361b43c294de1b1fa.

@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 16, 2015
@thockin
Refactor IntOrString into a new pkg
ba383bc 
pkg/util/intstr is a cleaner encapsulation for this type and supporting
functions.  No behavioral change.
@thockin thockin removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 16, 2015
@k8s-bot
Copy link

k8s-bot commented Nov 16, 2015

GCE e2e test build/test passed for commit ba383bc.

@thockin
Copy link
Member Author

thockin commented Nov 16, 2015

@k8s-bot unit test this please

1 similar comment
@thockin
Copy link
Member Author

thockin commented Nov 16, 2015

@k8s-bot unit test this please

@thockin thockin added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 17, 2015
@k8s-github-robot
Copy link

Continuous integration appears to have missed, closing and re-opening to trigger it

@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 Nov 17, 2015

GCE e2e test build/test passed for commit ba383bc.

@k8s-github-robot
Copy link

Automatic merge from submit-queue

k8s-github-robot pushed a commit that referenced this pull request Nov 17, 2015
Merge pull request #14469 from thockin/intstr
0c9bc32 
Auto commit by PR queue bot
@k8s-github-robot k8s-github-robot merged commit 0c9bc32 into kubernetes:master Nov 17, 2015
@bprashanth bprashanth mentioned this pull request Nov 18, 2015
Merged
@thockin thockin deleted the intstr branch January 4, 2016 04:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@derekwaynecarr derekwaynecarr

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/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@thockin @k8s-bot @k8s-github-robot @derekwaynecarr @brendandburns @k8s-teamcity-mesosphere @googlebot