-
Notifications
You must be signed in to change notification settings - Fork 747
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: Remove crashOnFailureFetchingExpectations flag #3453
fix: Remove crashOnFailureFetchingExpectations flag #3453
Conversation
Signed-off-by: David-Jaeyoon-Lee <davjlee@google.com>
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
…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: