added 'appsv1.DeploymentReplicaFailure' condition #878
Conversation
|
Hello @cppforlife Please review this PR. |
| @@ -59,6 +59,12 @@ func (s AppsV1Deployment) IsDoneApplying() DoneApplyState { | |||
| return DoneApplyState{Done: true, Successful: false, Message: fmt.Sprintf( | |||
| "Deployment failed to delete pods: %s (message: %s)", cond.Reason, cond.Message)} | |||
| } | |||
|
|
|||
| case "FailedCreate": | |||
There was a problem hiding this comment.
I just noticed that the switch is on the type of condition, and "FailedDelete" and "FailedCreate" are Reasons for failure for condition type "ReplicaFailure". So instead of "FailedDelete" above and "FailedCreate", we should be checking for appsv1.DeploymentReplicaFailure. (More details around status conditions can be found here)
It would be great if you could also add a unit test for this in apps_v1_deployment_test.go.
There was a problem hiding this comment.
Thank you so much for the explanation!!! I am working on this.
There was a problem hiding this comment.
I have added the appsv1.DeploymentReplicaFailure instead of failedcreate/failedDelete condition, also I found apps_v1_deployment_test.go had no test coverage for isDoneApplying function which contains these checks so I added an unit test covering both the checks inside the function ie one for depolyment progressing condition and other for replicafailure.
Signed-off-by: jignyasamishra <[email protected]>
jignyasamishra
force-pushed
the
jig/issue#350
branch
from
e97e7c6
to
43df63a
Compare
jignyasamishra
changed the title
There was a problem hiding this comment.
@jignyasamishra Changes are looking pretty good, I have left a couple of nit picky comments.
| @@ -65,3 +65,64 @@ func buildDep(resourcesBs string, t *testing.T) *ctlresm.AppsV1Deployment { | |||
|
|
|||
| return ctlresm.NewAppsV1Deployment(newResources[0], newResources[1:]) | |||
| } | |||
| func TestIsDoneApplying(t *testing.T) { | |||
There was a problem hiding this comment.
Let's move the test above the buildDep helper function. Also, let's rename it to TestAppsV1DeploymentReplicaFailure.
| @@ -65,3 +65,64 @@ func buildDep(resourcesBs string, t *testing.T) *ctlresm.AppsV1Deployment { | |||
|
|
|||
| return ctlresm.NewAppsV1Deployment(newResources[0], newResources[1:]) | |||
| } | |||
| func TestIsDoneApplying(t *testing.T) { | |||
| // Define a Deployment with a Progressing condition set to False | |||
There was a problem hiding this comment.
The code is self explanatory, so we can remove the comments.
| resources, err := ctlres.NewFileResource(ctlres.NewBytesSource([]byte(depYAML))).Resources() | ||
| require.NoError(t, err, "Expected resources to parse") | ||
|
|
||
| // Create an AppsV1Deployment instance with the parsed resources | ||
| deployment := ctlresm.NewAppsV1Deployment(resources[0], nil) |
There was a problem hiding this comment.
We can use the existing buildDep helper function for this.
| resources, err = ctlres.NewFileResource(ctlres.NewBytesSource([]byte(depYAML))).Resources() | ||
| require.NoError(t, err, "Expected resources to parse") | ||
| deployment = ctlresm.NewAppsV1Deployment(resources[0], nil) |
There was a problem hiding this comment.
Same as above, we can use the existing helper function.
There was a problem hiding this comment.
I have made the requested changes. Please have a look.
Signed-off-by: jignyasamishra <[email protected]>
Signed-off-by: jignyasamishra <[email protected]>
Thank you so much for your guidance!!! Please do let me know if i can help in some other issues. |
github-actions
bot
added
the
carvel triage
praveenrewar
removed
the
carvel triage
What this PR does / why we need it:
The PR enhances deployment logic by adding a "FailedCreate" condition. This helps identify and handle specific failures during creation, improving error handling in deployments.
Which issue(s) this PR fixes:
Fixes #350
Does this PR introduce a user-facing change?
Additional Notes for your reviewer:
Review Checklist:
a link to that PR
change
Additional documentation e.g., Proposal, usage docs, etc.: