Update log level to run tests on debug level #5071

taragu · 2019-08-06T13:24:13Z

/lint

Proposed Changes

Update log level to run tests on debug level

Release Note

NONE

knative-prow-robot · 2019-08-06T13:24:28Z

Hi @taragu. Thanks for your PR.

I'm waiting for a knative member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

markusthoemmes

This changes the default loglevel everywhere. I'm not certain that's what we want, do we?

markusthoemmes · 2019-08-06T13:51:46Z

It actually changes the level nowhere, because it's in the example block.

taragu · 2019-08-06T13:59:24Z

@markusthoemmes ahh got it. Is test/config/300-configmap.yaml what we want to change?

markusthoemmes · 2019-08-06T14:04:46Z

That configmap has nothing to do with loglevels I think? I don't know how to approach this issue best tbh. We'll beed to change what we install in the tests before we install it I guess.

markusthoemmes · 2019-08-07T18:03:26Z

test/config/config-logging.yaml

+    serving.knative.dev/release: devel
+
+data:
+  zap-logger-config: |


I think you can leave this out entirely to pick up the default? Not entirely sure though.

I just tried applying this yaml without this block and the zap-logger-config was not set

NewConfigFromConfigMap function will not pick up logging config without zap-logger-config key. @markusthoemmes were you talking about labels or the zap-logger-config key?

cc @andrew-su

My thought was that we'd only need the specific loglevels below and that the broader loggingconfig would be defaulted in.

If both are kubectl applied, then applying this without it will strip it in the three way merge.

shashwathi · 2019-08-08T21:47:02Z

/ok-to-test

mattmoor · 2019-08-09T15:00:37Z

test/config/config-logging.yaml

+apiVersion: v1
+kind: ConfigMap
+metadata:
+  name: config-logging


So for the main file, we symlink it under testdata and validate it's content in a Go test. Can we do the same here to validate changes? Otherwise this looks good to me

@mattmoor Sounds good. Should it be a new test under test/? Which package should it belong?

I'd just put it right next to where we test the real config (same test, second symlink)

@mattmoor sounds good! Just added the test and symlink

knative-test-reporter-robot · 2019-08-09T19:36:58Z

The following tests are currently flaky. Running them again to verify...

Test name	Retries
pull-knative-serving-integration-tests	1/3

Failed non-flaky tests preventing automatic retry of pull-knative-serving-integration-tests:

test/e2e.TestGRPCStreamingPingViaActivator

taragu · 2019-08-09T19:53:29Z

/test pull-knative-serving-integration-tests

taragu · 2019-08-12T16:00:39Z

@markusthoemmes @mattmoor this PR is ready for another review. Would you please take a look?

vagababov · 2019-08-12T16:20:36Z

pkg/logging/config_test.go

@@ -93,6 +95,56 @@ func TestOurConfig(t *testing.T) {
 	}
 }

+func TestLogLevelTestConfig(t *testing.T) {
+	wantCfg := `{


const here and the next one.

vagababov · 2019-08-12T16:22:54Z

pkg/logging/config_test.go

+		}
+	}
+	if got := cfg.LoggingConfig; got != wantCfg {
+		t.Errorf("LoggingConfig = %v, want %v, diff %s", got, wantCfg, cmp.Diff(got, wantCfg))


Suggested change

t.Errorf("LoggingConfig = %v, want %v, diff %s", got, wantCfg, cmp.Diff(got, wantCfg))

t.Errorf("LoggingConfig = %v, want %v, diff(-want +got) %s", got, wantCfg, cmp.Diff(wantCfg, got))

(https://godoc.org/github.com/google/go-cmp/cmp#Diff, most (but surely not all) follows this, see the Diff example)

mattmoor

/lgtm
/approve

knative-prow-robot · 2019-08-13T03:27:25Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mattmoor, taragu

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:

~~hack/OWNERS~~ [mattmoor]
~~pkg/logging/OWNERS~~ [mattmoor]
~~test/OWNERS~~ [mattmoor]

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

googlebot added the cla: yes Indicates the PR's author has signed the CLA. label Aug 6, 2019

knative-prow-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Aug 6, 2019

knative-prow-robot requested review from mattmoor and vaikas August 6, 2019 13:24

markusthoemmes reviewed Aug 6, 2019

View reviewed changes

taragu force-pushed the tests-log-level branch from b41ee2e to 08cd73a Compare August 6, 2019 13:58

knative-prow-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. area/test-and-release It flags unit/e2e/conformance/perf test issues for product features and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Aug 6, 2019

taragu force-pushed the tests-log-level branch from 08cd73a to bb2c97c Compare August 7, 2019 17:52

markusthoemmes reviewed Aug 7, 2019

View reviewed changes

mattmoor assigned mdemirhan Aug 7, 2019

knative-prow-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Aug 8, 2019

taragu force-pushed the tests-log-level branch from bb2c97c to 46b40d1 Compare August 9, 2019 14:27

mattmoor reviewed Aug 9, 2019

View reviewed changes

taragu force-pushed the tests-log-level branch from 46b40d1 to a29c45a Compare August 9, 2019 16:48

knative-prow-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. area/monitoring and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Aug 9, 2019

taragu force-pushed the tests-log-level branch 2 times, most recently from 58fd6a9 to dde6921 Compare August 9, 2019 19:02

Tara Gu added 2 commits August 12, 2019 11:59

Update log level to run tests on debug level

9ce6f33

Update hack/verify-codegen.sh for added symlink

db5296f

taragu force-pushed the tests-log-level branch from dde6921 to db5296f Compare August 12, 2019 15:59

vagababov reviewed Aug 12, 2019

View reviewed changes

Address review comments

6c558c8

mattmoor reviewed Aug 13, 2019

View reviewed changes

knative-prow-robot assigned mattmoor Aug 13, 2019

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Aug 13, 2019

knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 13, 2019

knative-prow-robot merged commit 2600036 into knative:master Aug 13, 2019

taragu deleted the tests-log-level branch November 12, 2019 16:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update log level to run tests on debug level #5071

Update log level to run tests on debug level #5071

taragu commented Aug 6, 2019

knative-prow-robot commented Aug 6, 2019

markusthoemmes left a comment

markusthoemmes commented Aug 6, 2019

taragu commented Aug 6, 2019

markusthoemmes commented Aug 6, 2019

markusthoemmes Aug 7, 2019

taragu Aug 7, 2019

shashwathi Aug 7, 2019

markusthoemmes Aug 9, 2019

mattmoor Aug 9, 2019

shashwathi commented Aug 8, 2019

mattmoor Aug 9, 2019

taragu Aug 9, 2019

mattmoor Aug 9, 2019

taragu Aug 9, 2019

knative-test-reporter-robot commented Aug 9, 2019

taragu commented Aug 9, 2019

taragu commented Aug 12, 2019

vagababov Aug 12, 2019

vagababov Aug 12, 2019

mattmoor left a comment

knative-prow-robot commented Aug 13, 2019

	t.Errorf("LoggingConfig = %v, want %v, diff %s", got, wantCfg, cmp.Diff(got, wantCfg))
	t.Errorf("LoggingConfig = %v, want %v, diff(-want +got) %s", got, wantCfg, cmp.Diff(wantCfg, got))

Update log level to run tests on debug level #5071

Update log level to run tests on debug level #5071

Conversation

taragu commented Aug 6, 2019

Proposed Changes

knative-prow-robot commented Aug 6, 2019

markusthoemmes left a comment

Choose a reason for hiding this comment

markusthoemmes commented Aug 6, 2019

taragu commented Aug 6, 2019

markusthoemmes commented Aug 6, 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

Choose a reason for hiding this comment

shashwathi commented Aug 8, 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

knative-test-reporter-robot commented Aug 9, 2019

taragu commented Aug 9, 2019

taragu commented Aug 12, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattmoor left a comment

Choose a reason for hiding this comment

knative-prow-robot commented Aug 13, 2019