Changed log.info to tidy up using fmt.sprintf #2246

bharathi-tenneti · 2019-11-20T17:15:05Z

Description of the change:
Modified log,Info in watches.go file to print message that doesn't necessarily state failure.

Motivation for the change:
The log should be info because it is not a failure but information

CLOSES: #2186

joelanford

@bharathi-tenneti @jmrodri @camilamacedo86 Do you think it would make sense to do this instead:

maxWorkers := defValue
if val, ok := os.LookupEnv(envVar); ok {
    if i, err := strconv.Atoi(val); err != nil {
        log.Info(fmt.Sprintf("Failed to parse %v from environment. Using default %v", envVar, defValue))
    } else {
        maxWorkers = i
    }
}
return maxWorkers

joelanford · 2019-11-20T17:50:21Z

pkg/ansible/watches/watches.go

@@ -275,7 +275,8 @@ func getMaxWorkers(gvk schema.GroupVersionKind, defValue int) int {
 	maxWorkers, err := strconv.Atoi(os.Getenv(envVar))
 	if err != nil {
 		// we don't care why we couldn't parse it just use default
-		log.Info("Failed to parse %v from environment. Using default %v", envVar, defValue)
+		//log.Info("Failed to parse %v from environment. Using default %v", envVar, defValue)


No need to keep this line around anymore.

Ditto for ansibleVerbosity.

joelanford · 2019-11-20T17:50:35Z

pkg/ansible/watches/watches.go

-		log.Info("Failed to parse %v from environment. Using default %v", envVar, defValue)
+		//log.Info("Failed to parse %v from environment. Using default %v", envVar, defValue)
+		log.Info(fmt.Sprintf("Using default value for ansible verbosity %d", defValue))
+


Nit: this extraneous empty line can be removed.

camilamacedo86

Just the same nits and all shows fine.
Good work 🥇

pkg/ansible/watches/watches.go

camilamacedo86 · 2019-11-21T00:19:02Z

+1 for the @joelanford suggestion.

Just the text that I'd recommend not use the word error or failed, keep the env var for the user know how to change that and the info. So, something as:

Unable to find a value for %v". Then, using the default value for the environment variable %d", envVar, defValue))

* leader election bugfix: Delete evicted leader pods Before this patch, when the leader pod is hard evicted but not deleted the leader lock configmap is not garbage collected and subsequent operator pods can never become leader. With this patch, an operator attempting to become the leader is able to delete evicted leader pods triggering garbage collection and allowing leader election to continue. Sometimes, evicted operator pods will remain, even with this patch. This occurs when the leader operator pod is evicted and a new operator pod is created on the same node. In this case, the new pod will also be evicted. When an operator pod is created on a non-failing node, leader election will delete only the evicted leader pod, leaving any evicted operator pods that were not the leader. To replicate the evicted state, I used a `kind` cluster with 2 worker nodes with altered kubelet configuration and a memory-hog version of the memcached operator. See the [altered operator docs](https://github.com/asmacdo/go-memcahced-operator/blob/explosive-operator/README.md)

… into watches-go

CHANGELOG.md

camilamacedo86 · 2019-11-21T18:56:31Z

go.mod

@@ -16,6 +16,7 @@ require (
 	github.com/elazarl/goproxy/ext v0.0.0-20190421051319-9d40249d3c2f // indirect
 	github.com/fatih/camelcase v1.0.0 // indirect


required revert the changes in the go.mod

joelanford · 2019-11-21T20:15:24Z

pkg/ansible/watches/watches.go

@@ -297,7 +294,7 @@ func getAnsibleVerbosity(gvk schema.GroupVersionKind, defValue int) int {
 	))
 	ansibleVerbosity, err := strconv.Atoi(os.Getenv(envVar))


I think we should use the same logic for the ansible verbosity operator.

Since the logic is identical, I think it would make sense to define a new function with this signature:

func getIntegerEnvWithDefault(envVar string, defValue int) int

The call it for both maxWorkers and ansibleVerbosity.

joelanford · 2019-11-21T20:29:12Z

pkg/ansible/watches/watches.go

+	maxWorkers := defValue
+	if val, ok := os.LookupEnv(envVar); ok {
+		if i, err := strconv.Atoi(val); err != nil {
+			log.Info(fmt.Sprintf("Unable to find a value for %v. Using default value for maxWorkers %d", envVar, defValue))


I could be wrong, but I think @camilamacedo86 was suggesting that we should log a separate message if we use the default when the environment variable is not set. I would also suggest we follow the structured logging conventions of the logr.Logger.

So here, we can use the following since we have encountered an error trying to parse the string as an integer.

log.Info("could not parse environment variable as an integer; using default value", "envVar", envVar, "default", defValue)

For @camilamacedo86's suggestion, if we do the os.LookupEnv() and ok == false, that means the environment variable was not set. In think in that case, we could log this message:

log.Info("environment variable not set; using default value", "envVar", envVar, "default", defValue)

@camilamacedo86 @jmrodri @bharathi-tenneti WDYT?

CHANGELOG.md

go.mod

CHANGELOG.md

joelanford · 2019-12-04T19:36:26Z

pkg/ansible/watches/watches.go

+		} else {
+			val = i
+		}
+	} else if ok==false {


Nit:

Suggested change

} else if ok==false {

} else if !ok {

joelanford

A couple more fixes. LGTM once sanity tests pass.

joelanford · 2019-12-06T02:56:30Z

pkg/ansible/watches/watches.go

@@ -312,3 +297,18 @@ func getAnsibleVerbosity(gvk schema.GroupVersionKind, defValue int) int {
 	}
 	return ansibleVerbosity
 }
+
+// Returns value for MaxWorkers/Ansibleverbosity based on if envVar is set or a defvalue is used.


Nit: Go doc convention is to use the identifier name (in this case the function name) as the first word of the comment. Not a huge deal since this function is unexported, but a good habit nonetheless.

joelanford · 2019-12-06T02:58:36Z

pkg/ansible/watches/watches.go

+			val = i
+		}
+	} else if !ok {
+ 		log.Info("environment variable not set; using default value", "envVar", envVar, "default", defValue)


Looks like the sanity test is complaining that these log.Info messages don't start with an upper case letter.

So we need:
Could not parse environment variable... and Environment variable not set...

camilamacedo86

/lgtm
as well.

camilamacedo86

Not passing in the sanity; https://travis-ci.org/operator-framework/operator-sdk/jobs/621627642#L844

joelanford · 2019-12-06T15:37:45Z

@bharathi-tenneti Looks like a go fmt issue. You should be able to run make format to fix it.

Most editors have a Go plugin that can be configured to automatically run go fmt on save, which I find pretty helpful.

camilamacedo86

/lgtm
/approved
when the CI issue be fixed

Tested locally:

openshift-ci-robot · 2019-12-09T15:42:02Z

New changes are detected. LGTM label has been removed.

Changed log.info to tidy up using fmt.sprintf

86981e0

openshift-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Nov 20, 2019

bharathi-tenneti requested a review from jmrodri November 20, 2019 17:15

openshift-ci-robot requested a review from fabianvf November 20, 2019 17:15

joelanford reviewed Nov 20, 2019

View reviewed changes

camilamacedo86 reviewed Nov 20, 2019

View reviewed changes

camilamacedo86 reviewed Nov 21, 2019

View reviewed changes

pkg/ansible/watches/watches.go Outdated Show resolved Hide resolved

camilamacedo86 reviewed Nov 21, 2019

View reviewed changes

pkg/ansible/watches/watches.go Outdated Show resolved Hide resolved

camilamacedo86 reviewed Nov 21, 2019

View reviewed changes

pkg/ansible/watches/watches.go Outdated Show resolved Hide resolved

camilamacedo86 reviewed Nov 21, 2019

View reviewed changes

pkg/ansible/watches/watches.go Outdated Show resolved Hide resolved

camilamacedo86 requested review from joelanford and camilamacedo86 November 21, 2019 00:19

camilamacedo86 added the kind/bug Categorizes issue or PR as related to a bug. label Nov 21, 2019

bharathi-tenneti changed the title ~~Changed log.info to tidy up using fmt.sprintf~~ WIP: Changed log.info to tidy up using fmt.sprintf Nov 21, 2019

openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 21, 2019

maxWorkers logic added for log.info

894f3cf

openshift-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 Nov 21, 2019

Bharathi Tenneti and others added 8 commits November 21, 2019 10:45

removed commented code

9b9bce9

Changed log.info to tidy up using fmt.sprintf

3d06649

feat(linter) : add goconst check and fix issues (#2236)

d34bf29

maxWorkers logic added for log.info

681e441

logic added

bd4d60a

2246 PR log addedin CHANGELOG file

d3870fd

Merge branch 'watches-go' of github.com:bharathi-tenneti/operator-sdk…

c0d9b5a

… into watches-go

camilamacedo86 reviewed Nov 21, 2019

View reviewed changes

CHANGELOG.md Show resolved Hide resolved

camilamacedo86 reviewed Nov 21, 2019

View reviewed changes

joelanford requested changes Nov 21, 2019

View reviewed changes

Rebased to master

cc4ef7c

camilamacedo86 reviewed Nov 22, 2019

View reviewed changes

go.mod Outdated Show resolved Hide resolved

Added getIntegerEnvWithDefault func

e2fc799

openshift-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Dec 4, 2019

Added CHANGELOG comment for PR2246

fa18fa6

camilamacedo86 reviewed Dec 4, 2019

View reviewed changes

go.mod Outdated Show resolved Hide resolved

Restored Go.mod with upstream

155020f

camilamacedo86 reviewed Dec 4, 2019

View reviewed changes

CHANGELOG.md Show resolved Hide resolved

joelanford reviewed Dec 4, 2019

View reviewed changes

Bharathi Tenneti added 2 commits December 5, 2019 10:31

Removed empty lines, ok==false changed

24d80f5

Restored go.mod, and go.sum

10fae31

joelanford approved these changes Dec 6, 2019

View reviewed changes

Fixed case for Log messages

59819b5

camilamacedo86 approved these changes Dec 6, 2019

View reviewed changes

openshift-ci-robot assigned camilamacedo86 Dec 6, 2019

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Dec 6, 2019

camilamacedo86 suggested changes Dec 6, 2019

View reviewed changes

Ran make format

f2c7fd3

openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Dec 6, 2019

camilamacedo86 approved these changes Dec 7, 2019

View reviewed changes

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Dec 7, 2019

maxWorkers<=0 check added

d906611

openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Dec 9, 2019

joelanford changed the title ~~WIP: Changed log.info to tidy up using fmt.sprintf~~ Changed log.info to tidy up using fmt.sprintf Dec 9, 2019

openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 9, 2019

bharathi-tenneti merged commit 82bc6e7 into operator-framework:master Dec 9, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Changed log.info to tidy up using fmt.sprintf #2246

Changed log.info to tidy up using fmt.sprintf #2246

bharathi-tenneti commented Nov 20, 2019 •

edited

Loading

joelanford left a comment

joelanford Nov 20, 2019

joelanford Nov 20, 2019

camilamacedo86 left a comment

camilamacedo86 commented Nov 21, 2019

camilamacedo86 Nov 21, 2019

joelanford Nov 21, 2019

joelanford Nov 21, 2019

joelanford Dec 4, 2019

joelanford left a comment

joelanford Dec 6, 2019

joelanford Dec 6, 2019

camilamacedo86 left a comment

camilamacedo86 left a comment

joelanford commented Dec 6, 2019

camilamacedo86 left a comment

openshift-ci-robot commented Dec 9, 2019

		@@ -16,6 +16,7 @@ require (
		github.com/elazarl/goproxy/ext v0.0.0-20190421051319-9d40249d3c2f // indirect
		github.com/fatih/camelcase v1.0.0 // indirect

		@@ -297,7 +294,7 @@ func getAnsibleVerbosity(gvk schema.GroupVersionKind, defValue int) int {
		))
		ansibleVerbosity, err := strconv.Atoi(os.Getenv(envVar))

Changed log.info to tidy up using fmt.sprintf #2246

Changed log.info to tidy up using fmt.sprintf #2246

Conversation

bharathi-tenneti commented Nov 20, 2019 • edited Loading

joelanford left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

camilamacedo86 left a comment

Choose a reason for hiding this comment

camilamacedo86 commented Nov 21, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joelanford left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

camilamacedo86 left a comment

Choose a reason for hiding this comment

camilamacedo86 left a comment

Choose a reason for hiding this comment

joelanford commented Dec 6, 2019

camilamacedo86 left a comment

Choose a reason for hiding this comment

openshift-ci-robot commented Dec 9, 2019

bharathi-tenneti commented Nov 20, 2019 •

edited

Loading