🐛 Ensure the `environment.InitFlags()` is called to avoid confusion. #269

cardil · 2021-12-23T21:05:16Z

Changes

🐛 Ensure the flags are valid

/kind bug

Fixes #268

Release Note

- :bug: Ensure the flags are valid

Also, moving code to TestMain() to avoid side effects, while importing a package.

cardil · 2022-01-04T11:30:05Z

/retest

cardil · 2022-01-11T12:06:46Z

/assign @dprotaso
/assign @julz
/assign @nak3
/assign @devguyio
/assign @lionelvillard
/assign @matzew
/assign @rhuss
/assign @chizhg

pkg/feature/level.go

dprotaso · 2022-01-11T13:57:46Z

test/example/main_test.go

@@ -38,13 +38,11 @@ import (
 // config as well as the parsing level and state flags.
 var global environment.GlobalEnvironment

-func init() {
+// TestMain is the first entry point for `go test`.
+func TestMain(m *testing.M) {
 	// environment.InitFlags registers state and level filter flags.
 	environment.InitFlags(flag.CommandLine)


I'm surprised InitFlags isn't instanced - ie. we use local variable vs. package ones global.InitFlags

Can you elaborate on this? I do not follow...

InitFlags is initializing package global variables in reconciler-test

When it could simply just modify values on the declared global environment in the example. At least reconciler-test vars aren't exposed publicly so maybe that's ok. 🤷

Yes. I agree such refactor would be beneficial. Mind opening an issue to track it?

…-flags

dprotaso · 2022-01-13T15:38:19Z

there's a conflict with HEAD

/lgtm
/approve

knative-prow-robot · 2022-01-13T15:38:26Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cardil, dprotaso

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~~ [dprotaso]

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

chizhg · 2022-03-23T17:12:58Z

/retest

…-flags Conflicts fixed: * README.md * pkg/feature/level.go * pkg/feature/states.go * test/e2e/main_test.go * test/example/main_test.go

knative-prow · 2022-06-22T16:02:18Z

New changes are detected. LGTM label has been removed.

knative-prow · 2022-06-22T16:02:24Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cardil, dprotaso

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~~ [cardil,dprotaso]

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

linux-foundation-easycla · 2022-06-22T16:02:28Z

The committers listed above are authorized under a signed CLA.

✅ login: cardil / name: Chris Suszynski (67679eb, 04b1218, 01b3c25, fb1418c, e388a31, 043e296)

cardil · 2022-07-06T13:57:35Z

@dprotaso Could you slap the PR again?

github-actions · 2022-10-22T01:33:41Z

This Pull Request is stale because it has been open for 90 days with
no activity. It will automatically close after 30 more days of
inactivity. Reopen with /reopen. Mark as fresh by adding the
comment /remove-lifecycle stale.

cardil · 2023-02-28T18:28:44Z

/reopen
/remove-lifecycle stale

knative-prow · 2023-02-28T18:28:47Z

@cardil: Reopened this PR.

In response to this:

/reopen
/remove-lifecycle stale

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.

…-flags

cardil · 2023-02-28T18:52:05Z

/cc @pierDipi

pkg/environment/magic.go

pierDipi · 2023-03-01T09:16:55Z

pkg/feature/level_test.go

+// levelPowerset returns all combinations for a given array.
+func levelPowerset(set []feature.Levels) (subsets [][]feature.Levels) {
+	length := uint(len(set))
+
+	// Go through all possible combinations of objects
+	// from 1 (only first object in subset) to 2^length (all objects in subset)
+	for subsetBits := 1; subsetBits < (1 << length); subsetBits++ {
+		var subset []feature.Levels
+
+		for object := uint(0); object < length; object++ {
+			// checks if object is contained in subset
+			// by checking if bit 'object' is set in subsetBits
+			if (subsetBits>>object)&1 == 1 {
+				// add object to subset
+				subset = append(subset, set[object])
+			}
+		}
+		// add subset to subsets
+		subsets = append(subsets, subset)
+	}
+	return subsets
+}
+
+func levelBitwiseOr(subsets [][]feature.Levels) []feature.Levels {
+	levels := make([]feature.Levels, len(subsets))
+	for i, subset := range subsets {
+		val := 0
+		for _, level := range subset {
+			val |= int(level)
+		}
+		levels[i] = feature.Levels(val)
+	}
+	return levels
+}


This is complex and hard to read code, what's the value of running all combinations when testing the boundaries is just enough?

This code computes the all bitwise combinations of all levels. For example: MUST|MUST_NOT|SHOULD or MAY|MUST or just MUST.

How about I add such doc to the test? WDYT?

I think this is solved in 7bd94c5

pierDipi · 2023-03-01T09:17:19Z

pkg/feature/states_test.go

+func statePowerset(set []feature.States) (subsets [][]feature.States) {
+	length := uint(len(set))
+
+	// Go through all possible combinations of objects
+	// from 1 (only first object in subset) to 2^length (all objects in subset)
+	for subsetBits := 1; subsetBits < (1 << length); subsetBits++ {
+		var subset []feature.States
+
+		for object := uint(0); object < length; object++ {
+			// checks if object is contained in subset
+			// by checking if bit 'object' is set in subsetBits
+			if (subsetBits>>object)&1 == 1 {
+				// add object to subset
+				subset = append(subset, set[object])
+			}
+		}
+		// add subset to subsets
+		subsets = append(subsets, subset)
+	}
+	return subsets
+}
+
+func stateBitwiseOr(subsets [][]feature.States) []feature.States {
+	levels := make([]feature.States, len(subsets))
+	for i, subset := range subsets {
+		val := 0
+		for _, state := range subset {
+			val |= int(state)
+		}
+		levels[i] = feature.States(val)
+	}
+	return levels
+}


Same here, very hard to read and follow

I think this is solved in 7bd94c5

pierDipi · 2023-03-01T09:21:33Z

pkg/feature/level.go

+// Valid checks if given level is valid (not empty combination of Levels values).
+func (l Levels) Valid() bool {
+	val := uint8(l)
+	if val == 0 {
+		return false
+	}
+	val &^= uint8(All)
+	return val == 0
+}


I'd like to reduce bit manipulation as much as possible, a simple:

levels := sets.NewInt32( int32(All), int32(Must), int32(MustNot), int32(Should), int32(ShouldNot), int32(May), ) return levels.Has(int32(l))

is much more readable and can go a long way

Same comment for Valid for States

It's not the same. Now, you could use bitwise or to select multiple levels or states.

Isn't this an input from command line? how do you do bitwise from command line?

That suggests that the type is wrong and shouldn't be a single value but rather an array

No. You could invoke any number of those levels and features. See the code:

https://github.com/knative-sandbox/reconciler-test/blob/0979798b59c810bcfc4cfa4f63f75bb8f5711020/pkg/environment/flags.go#L41-L56

Just call the tests with those flags or states:

-feature.alpha -feature.beta -requirement.must -requirement.should -requirement.may

Still, this suggests that an array to represents Levels and States is better

That's not the point of this PR, isn't?

Co-authored-by: Pierangelo Di Pilato <[email protected]>

…-flags

github-actions · 2023-06-02T01:36:22Z

This Pull Request is stale because it has been open for 90 days with
no activity. It will automatically close after 30 more days of
inactivity. Reopen with /reopen. Mark as fresh by adding the
comment /remove-lifecycle stale.

Ensure the environment.InitFlags() is called to avoid confusion.

67679eb

Also, moving code to TestMain() to avoid side effects, while importing a package.

knative-prow-robot added kind/bug Categorizes issue or PR as related to a bug. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Dec 23, 2021

Bump Github CI

04b1218

cardil mentioned this pull request Jan 5, 2022

🧹 Upgrade deps to K8s 0.22 knative-extensions/discovery#245

Merged

Bump Github CI

01b3c25

knative-prow-robot assigned chizhg, devguyio, dprotaso, julz, lionelvillard, matzew, nak3 and rhuss Jan 11, 2022

dprotaso reviewed Jan 11, 2022

View reviewed changes

pkg/feature/level.go Outdated Show resolved Hide resolved

dprotaso reviewed Jan 11, 2022

View reviewed changes

Work for all bit mask combinations

fb1418c

knative-prow-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jan 11, 2022

cardil requested a review from dprotaso January 11, 2022 19:46

Merge remote-tracking branch 'upstream/main' into bugfix/enforce-init…

e388a31

…-flags

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

knative-prow-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jan 13, 2022

cardil mentioned this pull request Jan 27, 2022

🐛 Eventshub receiver readiness ensured #284

Merged

Merge remote-tracking branch 'upstream/main' into bugfix/enforce-init…

043e296

…-flags Conflicts fixed: * README.md * pkg/feature/level.go * pkg/feature/states.go * test/e2e/main_test.go * test/example/main_test.go

knative-prow bot removed the lgtm Indicates that a PR is ready to be merged. label Jun 22, 2022

knative-prow-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 22, 2022

knative-prow-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 23, 2022

github-actions bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Oct 22, 2022

github-actions bot closed this Nov 21, 2022

knative-prow bot reopened this Feb 28, 2023

knative-prow bot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Feb 28, 2023

knative-prow-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 28, 2023

Merge remote-tracking branch 'upstream/main' into bugfix/enforce-init…

4cb6a34

…-flags

cardil force-pushed the bugfix/enforce-init-flags branch from f06f7cb to 4cb6a34 Compare February 28, 2023 18:50

knative-prow bot requested a review from pierDipi February 28, 2023 18:52

pierDipi reviewed Mar 1, 2023

View reviewed changes

cardil and others added 3 commits March 2, 2023 11:24

include current vals in failure

c199118

Co-authored-by: Pierangelo Di Pilato <[email protected]>

Explain the test code

7bd94c5

Merge remote-tracking branch 'upstream/main' into bugfix/enforce-init…

928a190

…-flags

cardil requested review from pierDipi and removed request for dprotaso March 2, 2023 11:45

Reformat

2242340

github-actions bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jun 2, 2023

github-actions bot closed this Jul 3, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🐛 Ensure the `environment.InitFlags()` is called to avoid confusion. #269

🐛 Ensure the `environment.InitFlags()` is called to avoid confusion. #269

cardil commented Dec 23, 2021 •

edited

Loading

cardil commented Jan 4, 2022

cardil commented Jan 11, 2022

dprotaso Jan 11, 2022 •

edited

Loading

cardil Jan 11, 2022

dprotaso Jan 11, 2022 •

edited

Loading

cardil Jan 11, 2022

dprotaso commented Jan 13, 2022

knative-prow-robot commented Jan 13, 2022

chizhg commented Mar 23, 2022

knative-prow bot commented Jun 22, 2022

knative-prow bot commented Jun 22, 2022

linux-foundation-easycla bot commented Jun 22, 2022 •

edited

Loading

cardil commented Jul 6, 2022

github-actions bot commented Oct 22, 2022

cardil commented Feb 28, 2023

knative-prow bot commented Feb 28, 2023

cardil commented Feb 28, 2023

pierDipi Mar 1, 2023

cardil Mar 2, 2023

cardil Mar 2, 2023

pierDipi Mar 1, 2023

cardil Mar 2, 2023

pierDipi Mar 1, 2023

cardil Mar 2, 2023

pierDipi Mar 2, 2023

cardil Mar 3, 2023

pierDipi Mar 3, 2023

cardil Mar 3, 2023

github-actions bot commented Jun 2, 2023

🐛 Ensure the environment.InitFlags() is called to avoid confusion. #269

🐛 Ensure the environment.InitFlags() is called to avoid confusion. #269

Conversation

cardil commented Dec 23, 2021 • edited Loading

Changes

cardil commented Jan 4, 2022

cardil commented Jan 11, 2022

dprotaso Jan 11, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dprotaso Jan 11, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dprotaso commented Jan 13, 2022

knative-prow-robot commented Jan 13, 2022

chizhg commented Mar 23, 2022

knative-prow bot commented Jun 22, 2022

knative-prow bot commented Jun 22, 2022

linux-foundation-easycla bot commented Jun 22, 2022 • edited Loading

cardil commented Jul 6, 2022

github-actions bot commented Oct 22, 2022

cardil commented Feb 28, 2023

knative-prow bot commented Feb 28, 2023

cardil commented Feb 28, 2023

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

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

Choose a reason for hiding this comment

github-actions bot commented Jun 2, 2023

🐛 Ensure the `environment.InitFlags()` is called to avoid confusion. #269

🐛 Ensure the `environment.InitFlags()` is called to avoid confusion. #269

cardil commented Dec 23, 2021 •

edited

Loading

dprotaso Jan 11, 2022 •

edited

Loading

dprotaso Jan 11, 2022 •

edited

Loading

linux-foundation-easycla bot commented Jun 22, 2022 •

edited

Loading