-
Notifications
You must be signed in to change notification settings - Fork 299
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
feat: mark programmed=false with an error when too many addresses are assigned #3713
Conversation
… assigned Signed-off-by: sanposhiho <[email protected]>
Signed-off-by: Kensei Nakada <[email protected]>
ceca34a
to
e3c93ac
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3713 +/- ##
==========================================
+ Coverage 68.81% 69.01% +0.20%
==========================================
Files 175 176 +1
Lines 21525 21746 +221
==========================================
+ Hits 14812 15009 +197
- Misses 5636 5657 +21
- Partials 1077 1080 +3 ☔ View full report in Codecov by Sentry. |
Signed-off-by: Kensei Nakada <[email protected]>
2d703fc
to
deb57ac
Compare
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.
followup comments
@@ -128,26 +129,45 @@ func computeGatewayAcceptedCondition(gw *gwapiv1.Gateway, accepted bool) metav1. | |||
} | |||
} | |||
|
|||
const ( |
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.
define messages as const to make it easier to be referred from test cases.
// computeGatewayProgrammedCondition computes the Gateway Programmed status condition. | ||
// Programmed condition surfaces true when the Envoy Deployment status is ready. | ||
func computeGatewayProgrammedCondition(gw *gwapiv1.Gateway, deployment *appsv1.Deployment) metav1.Condition { | ||
func computeGatewayProgrammedCondition(gw *gwapiv1.Gateway, deployment *appsv1.Deployment) { |
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.
interface has to be changed so that it can mutate gateway, not only condition.
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.
if that case lets call this updateGatewayProgrammedCondition
?
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.
That sounds better. Updated. 🙏
expect: metav1.Condition{ | ||
Status: metav1.ConditionFalse, | ||
Reason: string(gwapiv1.GatewayReasonNoResources), | ||
name: "not ready gateway with too many addresses", |
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.
Looks like too many diff in TestGatewayReadyCondition
though, the core change is the addition of this test case only.
Signed-off-by: Kensei Nakada <[email protected]>
6aa8352
to
6053799
Compare
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 thanks !
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
/retest |
1 similar comment
/retest |
What type of PR is this?
What this PR does / why we need it:
This PR changes the impl to mark programmed=false with an error when too many addresses are assigned.
The motivation here is to enhance the visibility of this error to users.
Which issue(s) this PR fixes:
Fixes #3662