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

Convert bool to error, helper func for cd to skew #30321

Merged
merged 1 commit into from
Sep 9, 2016
Merged

Convert bool to error, helper func for cd to skew #30321

merged 1 commit into from
Sep 9, 2016

Conversation

fejta
Copy link
Contributor

@fejta fejta commented Aug 9, 2016 •
edited by k8s-oncall
Loading

Switch from various functions returning False when something bad happens to returning an error
Encapsulate logic to switch to the skew directory inside chdirSkew

Also add a TODO for using hyphen-flags instead of underscore_flags

This change is Reviewable

@fejta fejta assigned krousey and ixdy Aug 9, 2016
@fejta fejta added the release-note-none Denotes a PR that doesn't merit a release note. label Aug 9, 2016
@k8s-github-robot k8s-github-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Aug 9, 2016
@fejta fejta force-pushed the check-resources branch 2 times, most recently from 56ffb66 to 131d83d Compare August 10, 2016 00:56
@fejta
Copy link
Contributor Author

fejta commented Aug 11, 2016

Can a brother get a code review @ixdy @krousey?

krousey
krousey reviewed Aug 11, 2016
View reviewed changes
hack/e2e.go

func doOrDie(msg string, err error) {
if err != nil {
log.Fatalf("error %s: %s", msg, err)
Copy link
Contributor

Choose a reason for hiding this comment

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

Please don't do this. I know it sucks to repeat code, but please log.Fatal where the error is received and not in a utility method like this. What you have here will always die on line 456. https://play.golang.org/p/SdbuTBrh8P

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

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

fejta commented Aug 19, 2016

@krousey thanks for the comments. PTAL when you have the chance!

@k8s-github-robot k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 19, 2016
@fejta
Copy link
Contributor Author

fejta commented Aug 20, 2016

@k8s-bot e2e test this issue #28275

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

krousey commented Aug 22, 2016

A couple more minor nits. Feel free to fix and add lgtm.

Reviewed 2 of 2 files at r2.
Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.

hack/e2e.go, line 202 [r2] (raw file):

  ap := filepath.Join(location, "gcp-resources-after.txt")
  var mode os.FileMode
  mode = 0664

This assignment can be on the same line. var mode os.FileMode = 0644

hack/e2e.go, line 207 [r2] (raw file):

      return err
  }
  if err := ioutil.WriteFile(filepath.Join(location, "gcp-resources-cluster-up.txt"), clusterUp, mode); err != nil {

maybe a variable for this path too?

Comments from Reviewable

@fejta
Copy link
Contributor Author

fejta commented Aug 23, 2016

Thanks!

Review status: 1 of 2 files reviewed at latest revision, 2 unresolved discussions.

hack/e2e.go, line 202 [r2] (raw file):

Previously, krousey wrote…

This assignment can be on the same line. var mode os.FileMode = 0644

Done.

hack/e2e.go, line 207 [r2] (raw file):

Previously, krousey wrote…

maybe a variable for this path too?

Done.

Comments from Reviewable

@k8s-github-robot k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 23, 2016
@fejta
Copy link
Contributor Author

fejta commented Aug 23, 2016

@k8s-bot e2e test this issue #30687

@fejta
Copy link
Contributor Author

fejta commented Aug 25, 2016

@k8s-bot gke e2e test this issue #27462

ixdy
ixdy reviewed Aug 26, 2016
View reviewed changes
hack/e2e.go
ap := writeOrDie(location, "gcp-resources-after.txt", after)

var mode os.FileMode = 0664
bp := filepath.Join(location, "gcp-resources-before.txt")
Copy link
Member

Choose a reason for hiding this comment

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

you have taken the Go unnecessarily-short-variable-name style to heart, I see. :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ack

@k8s-bot
Copy link

k8s-bot commented Aug 29, 2016

GCE e2e build/test passed for commit fd15911.

@fejta
Copy link
Contributor Author

fejta commented Aug 29, 2016

@k8s-bot node e2e test this issue #31633

@fejta fejta added lgtm "Looks good to me", indicates that a PR is ready to be merged. retest-not-required labels Sep 9, 2016
@fejta
Copy link
Contributor Author

fejta commented Sep 9, 2016

Adding lgtm per comments and retest-not-required as we do not use the head version of hack/e2e.go in testing.

@k8s-github-robot
Copy link

Automatic merge from submit-queue

@k8s-github-robot k8s-github-robot merged commit 08b6eaf into kubernetes:master Sep 9, 2016
xingzhou pushed a commit to xingzhou/kubernetes that referenced this pull request Dec 15, 2016
Merge pull request kubernetes#30321 from fejta/check-resources
8ff292e 
Automatic merge from submit-queue

Convert bool to error, helper func for cd to skew

Switch from various functions returning `False` when something bad happens to returning an `error`
Encapsulate logic to switch to the skew directory inside chdirSkew

Also add a TODO for using `hyphen-flags` instead of `underscore_flags`
@fejta fejta deleted the check-resources branch April 8, 2017 21:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@krousey krousey

@ixdy ixdy

Labels
lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note-none Denotes a PR that doesn't merit a release note. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@fejta @krousey @k8s-bot @k8s-github-robot @ixdy @googlebot