fix: Remove crashOnFailureFetchingExpectations flag #3453
Conversation
Signed-off-by: David-Jaeyoon-Lee <davjlee@google.com>
David-Jaeyoon-Lee
force-pushed
the
davjlee/refactor/ready-tracker-flag
branch
from
979c50b
to
c340a30
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #3453 +/- ##
==========================================
- Coverage 54.49% 48.08% -6.42%
==========================================
Files 134 219 +85
Lines 12329 14844 +2515
==========================================
+ Hits 6719 7137 +418
- Misses 5116 6903 +1787
- Partials 494 804 +310
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
The value of the flag is moot so we want to hide it from the user. Signed-off-by: David-Jaeyoon-Lee <davjlee@google.com>
| var crashOnFailureFetchingExpectations = flag.Bool("crash-on-failure-fetching-expectations", false, "Unless set (defaults to false), gatekeeper will ignore errors when gathering expectations. This prevents bootstrapping errors from crashing Gatekeeper at the cost of increasing the risk Gatekeeper will under-enforce policy. Enabling this will help prevent under-enforcement at the risk of crashing during startup. Note that enabling this flag currently does not achieve the aforementioned effect since fetching expectations will retry until success.") | ||
| // Commenting out the flag and replacing with a false boolean constant because the value of the flag is currently moot without a retry limit | ||
| // var crashOnFailureFetchingExpectations = flag.Bool("crash-on-failure-fetching-expectations", false, "Unless set (defaults to false), gatekeeper will ignore errors when gathering expectations. This prevents bootstrapping errors from crashing Gatekeeper at the cost of increasing the risk Gatekeeper will under-enforce policy. Enabling this will help prevent under-enforcement at the risk of crashing during startup. Note that enabling this flag currently does not achieve the aforementioned effect since fetching expectations will retry until success.") | ||
| const crashOnFailureFetchingExpectations = false |
There was a problem hiding this comment.
Can't do &false. I could do something like
var falseValue = false
var crashOnFailureFetchingExpectations = &falseValue, but seemed unnecessary
pkg/readiness/ready_tracker.go
Outdated
| @@ -90,7 +91,8 @@ type Tracker struct { | |||
|
|
|||
| // NewTracker creates a new Tracker and initializes the internal trackers. | |||
| func NewTracker(lister Lister, mutationEnabled, externalDataEnabled, expansionEnabled bool) *Tracker { | |||
| return newTracker(lister, mutationEnabled, externalDataEnabled, expansionEnabled, *crashOnFailureFetchingExpectations, nil, nil) | |||
| // Dereference crashOnFailureFetchingExpectations when we change crashOnFailureFetchingExpectations back to a flag | |||
There was a problem hiding this comment.
As a result of how I set crashOnFailureFetchingExpectations above, I added a comment to change this when we switch back to a flag, but maybe I need to add a TODO? not sure if just a comment is sufficient. @maxsmythe
There was a problem hiding this comment.
Add a TODO as a comment and open issue to track so we remember to add the flag back would be great!
There was a problem hiding this comment.
Done. Is a plain TODO sufficient or is there a way to reference/embed an issue to it. Here is the issue I created: #3460
When we support retry count on fetching expectations we want to expose the crashOnFailureFetchingExpectations as a flag instead of a constant. Signed-off-by: David-Jaeyoon-Lee <davjlee@google.com>
ritazh
changed the title
ritazh
changed the title
…t#3453) Signed-off-by: David-Jaeyoon-Lee <davjlee@google.com> Co-authored-by: Rita Zhang <rita.z.zhang@gmail.com>
What this PR does / why we need it:
Remove the flag crashOnFailureFetchingExpectations.
Special notes for your reviewer: