Read default regexes from configmap for tests. #1548

abutcher · 2021-09-23T21:12:11Z

2uasimojo · 2021-09-23T21:29:22Z

pkg/controller/clusterprovision/installlogmonitor_test.go

+// buildRegexConfigMap reads the install log regexes configmap from within config/configmaps/install-log-regexes-configmap.yaml
+func buildRegexConfigMap() runtime.Object {
+	decode := serializer.NewCodecFactory(scheme.Scheme).UniversalDeserializer().Decode
+	stream, err := ioutil.ReadFile("../../../config/configmaps/install-log-regexes-configmap.yaml")


Let's bindata this puppy.

Oh, look, it already is.

To be clear, I'm asking you to use the bindata asset instead of loading up the file "by hand" here.

bindata means we need to make update every time and it's not always clear why my changes do not come up. since go test runs from the source code directly, it's imo better to use the file directly.

Our verify CI test already enforces that bindata is in sync.

This is what bindata is for.

I could see the workflow of adding a new regex/test being a little confusing if you have to make bindata before running this test specifically. I click run test in vscode a lot.

Fair enough.

codecov · 2021-09-24T00:12:13Z

Codecov Report

Merging #1548 (b67dcd1) into master (22c6da1) will increase coverage by 0.06%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #1548      +/-   ##
==========================================
+ Coverage   41.50%   41.56%   +0.06%     
==========================================
  Files         336      336              
  Lines       30691    30885     +194     
==========================================
+ Hits        12737    12837     +100     
- Misses      16867    16956      +89     
- Partials     1087     1092       +5

Impacted Files	Coverage Δ
pkg/operator/assets/bindata.go	`0.00% <0.00%> (ø)`
pkg/controller/dnszone/dnszone_controller.go	`28.88% <0.00%> (ø)`
...b.com/openshift/hive/apis/hive/v1/dnszone_types.go	`100.00% <0.00%> (ø)`
...m/openshift/hive/apis/hive/v1/clusterpool_types.go	`100.00% <0.00%> (ø)`
...shift/hive/apis/hive/v1/clusterdeployment_types.go	`100.00% <0.00%> (ø)`
pkg/test/clusterdeployment/clusterdeployment.go	`96.66% <0.00%> (+0.07%)`	⬆️
pkg/test/clusterpool/clusterpool.go	`97.53% <0.00%> (+0.09%)`	⬆️
...g/controller/hibernation/hibernation_controller.go	`59.58% <0.00%> (+0.13%)`	⬆️
.../clusterdeployment/clusterdeployment_controller.go	`59.83% <0.00%> (+0.14%)`	⬆️
pkg/controller/clusterpool/collections.go	`76.88% <0.00%> (+0.27%)`	⬆️
... and 3 more

abutcher · 2021-09-24T14:15:28Z

/hold cancel

2uasimojo

There are several other test cases with inlined configmaps. If those regexes aren't part of the official config, do we still need to test them?

2uasimojo · 2021-09-24T17:03:45Z

pkg/controller/clusterprovision/installlogmonitor_test.go

-			}},
+			name:           "GCP compute quota",
+			log:            pointer.StringPtr(gcpCPUQuotaLog),
+			existing:       []runtime.Object{buildRegexConfigMap()},


This is latent, so could be handled in a separate PR, but is there any reason to build this on every test case? It's read-only, right?

There are some test cases that want a missing config or a malformed regex so that's why I have it per test case atm.

Could split them in a separate PR

some test cases that want a missing config or a malformed regex

So there are two different issues here.

This comment was about all test cases where existing is simply using buildRegexConfigMap(). All of those test cases should be able to reuse a common variable built once at the top of the function. That's the bit I'm saying was latent and could be handled in a separate PR.

Some of the test cases don't use buildRegexConfigMap() but instead use inlined configmaps. Are all of those specifically testing edge cases with missing/bad configmaps?

abutcher · 2021-09-24T17:21:20Z

If this goes before #1539 @yithian will have to rebase again so
/hold

2uasimojo

In any case, this is a good start. We can address further cleanup in subsequent PRs.

/lgtm

openshift-ci · 2021-09-24T17:26:28Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 2uasimojo, abutcher

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:

~~OWNERS~~ [2uasimojo,abutcher]

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

gregsheremeta · 2021-09-28T16:51:38Z

/unhold

2uasimojo · 2021-09-28T21:52:08Z

/test e2e-pool

Read default regexes from configmap for test.

b67dcd1

openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 23, 2021

openshift-ci bot requested review from abhinavdahiya and suhanime September 23, 2021 21:12

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 23, 2021

2uasimojo reviewed Sep 23, 2021

View reviewed changes

openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 24, 2021

2uasimojo reviewed Sep 24, 2021

View reviewed changes

openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 24, 2021

2uasimojo reviewed Sep 24, 2021

View reviewed changes

openshift-ci bot assigned 2uasimojo Sep 24, 2021

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Sep 24, 2021

openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 28, 2021

openshift-merge-robot merged commit e1182a0 into openshift:master Sep 28, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Read default regexes from configmap for tests. #1548

Read default regexes from configmap for tests. #1548

abutcher commented Sep 23, 2021

2uasimojo Sep 23, 2021

2uasimojo Sep 23, 2021

2uasimojo Sep 23, 2021

abhinavdahiya Sep 23, 2021 •

edited

2uasimojo Sep 24, 2021

abutcher Sep 24, 2021 •

edited

2uasimojo Sep 24, 2021

codecov bot commented Sep 24, 2021 •

edited

abutcher commented Sep 24, 2021

2uasimojo left a comment

2uasimojo Sep 24, 2021

abutcher Sep 24, 2021

abutcher Sep 24, 2021

2uasimojo Sep 24, 2021

abutcher commented Sep 24, 2021

2uasimojo left a comment

openshift-ci bot commented Sep 24, 2021

gregsheremeta commented Sep 28, 2021

2uasimojo commented Sep 28, 2021

Read default regexes from configmap for tests. #1548

Read default regexes from configmap for tests. #1548

Conversation

abutcher commented Sep 23, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abhinavdahiya Sep 23, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abutcher Sep 24, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Sep 24, 2021 • edited

Codecov Report

abutcher commented Sep 24, 2021

2uasimojo left a 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

Choose a reason for hiding this comment

abutcher commented Sep 24, 2021

2uasimojo left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Sep 24, 2021

gregsheremeta commented Sep 28, 2021

2uasimojo commented Sep 28, 2021

abhinavdahiya Sep 23, 2021 •

edited

abutcher Sep 24, 2021 •

edited

codecov bot commented Sep 24, 2021 •

edited