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

Log Errors Missing in Internal #3016

Merged
merged 1 commit into from
Sep 5, 2018
Merged

Log Errors Missing in Internal #3016

merged 1 commit into from
Sep 5, 2018

Conversation

diazjf
Copy link

@diazjf diazjf commented Aug 30, 2018

Adds a few missing errors and cleans some of the error log convention.

Fixes #3013

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Aug 30, 2018
antoineco
antoineco suggested changes Aug 30, 2018
View reviewed changes
internal/ingress/controller/store/store.go Outdated
@@ -159,7 +159,7 @@ func (i *Informer) Run(stopCh chan struct{}) {
i.Secret.HasSynced,
i.ConfigMap.HasSynced,
) {
runtime.HandleError(fmt.Errorf("Timed out waiting for caches to sync"))
runtime.HandleError(fmt.Errorf("timed out waiting for caches to sync"))
Copy link
Contributor

Choose a reason for hiding this comment

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

The capital letter is by design here. See logError

internal/ingress/controller/store/store.go Outdated
@@ -173,7 +173,7 @@ func (i *Informer) Run(stopCh chan struct{}) {
if !cache.WaitForCacheSync(stopCh,
i.Ingress.HasSynced,
) {
runtime.HandleError(fmt.Errorf("Timed out waiting for caches to sync"))
runtime.HandleError(fmt.Errorf("timed out waiting for caches to sync"))
Copy link
Contributor

Choose a reason for hiding this comment

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

The capital letter is by design here. See logError

internal/file/file.go Outdated
@@ -27,6 +28,7 @@ func SHA1(filename string) string {
hasher := sha1.New()
s, err := ioutil.ReadFile(filename)
if err != nil {
glog.Errorf("error reading file %v", err)
Copy link
Contributor

Choose a reason for hiding this comment

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

Should start with a capital letter.

@k8s-ci-robot k8s-ci-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Aug 30, 2018
@diazjf
Copy link
Author

diazjf commented Aug 30, 2018

@antoineco reverted the logs back to capital.

@codecov-io
Copy link

codecov-io commented Aug 30, 2018 •
edited
Loading

Codecov Report

Merging #3016 into master will increase coverage by <.01%.
The diff coverage is 33.33%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #3016      +/-   ##
=========================================
+ Coverage    47.5%   47.5%   +<.01%     
=========================================
  Files          77      77              
  Lines        5644    5646       +2     
=========================================
+ Hits         2681    2682       +1     
- Misses       2609    2611       +2     
+ Partials      354     353       -1
Impacted Files Coverage Δ
internal/ingress/controller/store/backend_ssl.go 43% <0%> (ø) ⬆️
internal/ingress/controller/controller.go 2.18% <0%> (ø) ⬆️
internal/ingress/controller/nginx.go 16.04% <0%> (ø) ⬆️
internal/k8s/main.go 84.61% <100%> (+0.4%) ⬆️
internal/file/file.go 100% <100%> (ø) ⬆️
internal/watch/file_watcher.go 80.76% <0%> (-3.85%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 05025d6...10de8ca. Read the comment docs.

@antoineco
Copy link
Contributor

All of them? Only 2 of them had the wrong format.

@aledbf aledbf added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 30, 2018
@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Aug 30, 2018
@diazjf
Copy link
Author

diazjf commented Aug 30, 2018

@antoineco resolved the issue:

I should have read https://github.com/golang/go/wiki/CodeReviewComments#error-strings

@antoineco
Copy link
Contributor

I did not even know this convention was documented :) Something learned!

antoineco
antoineco suggested changes Aug 30, 2018
View reviewed changes
Copy link
Contributor

@antoineco antoineco left a comment

Choose a reason for hiding this comment

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

Apart from this last comment of mine, LGTM

internal/k8s/main.go Outdated
@@ -42,6 +42,7 @@ func ParseNameNS(input string) (string, string, error) {
func GetNodeIPOrName(kubeClient clientset.Interface, name string, useInternalIP bool) string {
node, err := kubeClient.CoreV1().Nodes().Get(name, metav1.GetOptions{})
if err != nil {
glog.Error(err)
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd prepend some context like you did in internal/file/file.go. Eg. "Error getting node %s: %s" , name, err

Copy link
Author

Choose a reason for hiding this comment

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

Thanks will do!

@antoineco
Copy link
Contributor

/lgtm

@k8s-ci-robot k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Aug 30, 2018
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 30, 2018
@antoineco
Copy link
Contributor

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 30, 2018
Log Errors Missing in Internal
10de8ca 
Adds a few missing errors and fix formatting for others.

Fixes kubernetes#3013
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 30, 2018
@aledbf aledbf removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 31, 2018
@aledbf
Copy link
Member

aledbf commented Sep 5, 2018

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 5, 2018
@aledbf
Copy link
Member

aledbf commented Sep 5, 2018

@diazjf thanks!

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aledbf, antoineco, diazjf

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot merged commit 3f6314a into kubernetes:master Sep 5, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@antoineco antoineco antoineco requested changes

Assignees

@aledbf aledbf

@antoineco antoineco

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@diazjf @codecov-io @antoineco @aledbf @k8s-ci-robot