KAAP-851: Fixed the unit tests for Installation and Uninstallation Secret #65
Conversation
hsri-pf9
requested review from
cruizen,
jayanth-tjvrr,
mridulgain,
sebastian-pf9 and
vaibhavd21
Code Review Agent Run #15e4cbActionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
Changelist by BitoThis pull request implements the following key changes.
|
sebastian-pf9
left a comment
sebastian-pf9
left a comment
There was a problem hiding this comment.
lgtm, but I have no deep insights if this makes sense
agent/reconciler/reconciler_test.go
Outdated
| byoHostLookupKey types.NamespacedName | ||
| bootstrapSecret *corev1.Secret | ||
| installationSecret *corev1.Secret | ||
| uninstallationSecret *corev1.Secret // <-- add this |
There was a problem hiding this comment.
| uninstallationSecret *corev1.Secret // <-- add this | |
| uninstallationSecret *corev1.Secret |
Code Review Agent Run #e06a4aActionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
Code Review Agent Run #8ed0b0Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
|
Bito Review Skipped - No Changes Detected |
![bito-code-review[bot]](https://avatars.githubusercontent.com/in/1061978?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review Agent Run #158647
Actionable Suggestions - 1
-
agent/host_agent_suite_test.go - 1
- Incorrect user name in kubeconfig access · Line 156-161
Review Details
-
Files reviewed - 1 · Commit Range:
883e96b..93f19bd- agent/host_agent_suite_test.go
-
Files skipped - 0
-
Tools
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
Bito Usage Guide
Commands
Type the following command in the pull request comment and save the comment.
-
/review- Manually triggers a full AI review. -
/pause- Pauses automatic reviews on this pull request. -
/resume- Resumes automatic reviews. -
/resolve- Marks all Bito-posted review comments as resolved. -
/abort- Cancels all in-progress reviews.
Refer to the documentation for additional commands.
Configuration
This repository uses Default Agent You can customize the agent settings here or contact your Bito workspace admin at [email protected].
Documentation & Help
AI Code Review powered by
| configObj, err := clientcmd.LoadFromFile(getKubeConfig().Name()) | ||
| Expect(err).NotTo(HaveOccurred()) | ||
| userObj := configObj.AuthInfos["envtest"] | ||
| printDecodedPEM := func(field string, data []byte) { | ||
| decoded, err := base64.StdEncoding.DecodeString(string(data)) | ||
| if err != nil { |
There was a problem hiding this comment.
The code assumes the user name is 'envtest' but it's actually 'envtest-admin' as set in line 135. This will cause the PEM decoding to fail as it's looking for the wrong user in the kubeconfig.
Code suggestion
Check the AI-generated fix before applying
| configObj, err := clientcmd.LoadFromFile(getKubeConfig().Name()) | |
| Expect(err).NotTo(HaveOccurred()) | |
| userObj := configObj.AuthInfos["envtest"] | |
| printDecodedPEM := func(field string, data []byte) { | |
| decoded, err := base64.StdEncoding.DecodeString(string(data)) | |
| if err != nil { | |
| configObj, err := clientcmd.LoadFromFile(getKubeConfig().Name()) | |
| Expect(err).NotTo(HaveOccurred()) | |
| userObj := configObj.AuthInfos["envtest-admin"] | |
| printDecodedPEM := func(field string, data []byte) { | |
| decoded, err := base64.StdEncoding.DecodeString(string(data)) | |
| if err != nil { |
Code Review Run #158647
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
Code Review Agent Run #bd2daeActionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
|
Agent Unit tests passed, Now webhook unit tests are remainig which I am working on it. |
There was a problem hiding this comment.
Code Review Agent Run #d14744
Actionable Suggestions - 2
-
agent/reconciler/host_reconciler.go - 2
- Missing existence check before creating uninstall secret · Line 188-205
- Secret deleted before node reset operation completes · Line 313-325
Review Details
-
Files reviewed - 2 · Commit Range:
0163ff2..357f2c2- agent/reconciler/host_reconciler.go
- agent/reconciler/reconciler_test.go
-
Files skipped - 0
-
Tools
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
Bito Usage Guide
Commands
Type the following command in the pull request comment and save the comment.
-
/review- Manually triggers a full AI review. -
/pause- Pauses automatic reviews on this pull request. -
/resume- Resumes automatic reviews. -
/resolve- Marks all Bito-posted review comments as resolved. -
/abort- Cancels all in-progress reviews.
Refer to the documentation for additional commands.
Configuration
This repository uses Default Agent You can customize the agent settings here or contact your Bito workspace admin at [email protected].
Documentation & Help
AI Code Review powered by
| if uninstallOk { | ||
| uninstallSecretName := "byoh-uninstall-" + byoHost.Name | ||
| uninstallSecret := &corev1.Secret{ | ||
| ObjectMeta: metav1.ObjectMeta{ | ||
| Name: uninstallSecretName, | ||
| Namespace: byoHost.Namespace, | ||
| }, | ||
| Data: map[string][]byte{ | ||
| "uninstall": uninstallScriptBytes, | ||
| }, | ||
| } | ||
| if err := r.Client.Create(ctx, uninstallSecret); err != nil { | ||
| logger.Error(err, "error creating uninstall secret") | ||
| return err | ||
| } | ||
| byoHost.Spec.UninstallationSecret = &corev1.ObjectReference{ | ||
| Name: uninstallSecretName, | ||
| Namespace: byoHost.Namespace, |
There was a problem hiding this comment.
The code creates a new uninstall secret even if one might already exist with the same name. This could lead to errors if the secret already exists. Consider checking if the secret exists before creating it.
Code suggestion
Check the AI-generated fix before applying
| if uninstallOk { | |
| uninstallSecretName := "byoh-uninstall-" + byoHost.Name | |
| uninstallSecret := &corev1.Secret{ | |
| ObjectMeta: metav1.ObjectMeta{ | |
| Name: uninstallSecretName, | |
| Namespace: byoHost.Namespace, | |
| }, | |
| Data: map[string][]byte{ | |
| "uninstall": uninstallScriptBytes, | |
| }, | |
| } | |
| if err := r.Client.Create(ctx, uninstallSecret); err != nil { | |
| logger.Error(err, "error creating uninstall secret") | |
| return err | |
| } | |
| byoHost.Spec.UninstallationSecret = &corev1.ObjectReference{ | |
| Name: uninstallSecretName, | |
| Namespace: byoHost.Namespace, | |
| if uninstallOk { | |
| uninstallSecretName := "byoh-uninstall-" + byoHost.Name | |
| // Check if secret already exists | |
| existingSecret := &corev1.Secret{} | |
| err := r.Client.Get(ctx, types.NamespacedName{ | |
| Name: uninstallSecretName, | |
| Namespace: byoHost.Namespace, | |
| }, existingSecret) | |
| if err != nil && !errors.IsNotFound(err) { | |
| logger.Error(err, "error checking for existing uninstall secret") | |
| return err | |
| } | |
| if errors.IsNotFound(err) { | |
| // Create new secret if it doesn't exist | |
| uninstallSecret := &corev1.Secret{ | |
| ObjectMeta: metav1.ObjectMeta{ | |
| Name: uninstallSecretName, | |
| Namespace: byoHost.Namespace, | |
| }, | |
| Data: map[string][]byte{ | |
| "uninstall": uninstallScriptBytes, | |
| }, | |
| } | |
| if err := r.Client.Create(ctx, uninstallSecret); err != nil { | |
| logger.Error(err, "error creating uninstall secret") | |
| return err | |
| } | |
| } | |
| byoHost.Spec.UninstallationSecret = &corev1.ObjectReference{ | |
| Name: uninstallSecretName, | |
| Namespace: byoHost.Namespace, |
Code Review Run #d14744
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
| if err := r.Client.Delete(ctx, secret); err != nil { | ||
| logger.Error(err, "error deleting uninstallation secret") | ||
| return err | ||
| } | ||
| } else { | ||
| logger.Info("Skipping uninstallation of k8s components") | ||
| } | ||
|
|
||
| err := r.resetNode(ctx, byoHost) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
There was a problem hiding this comment.
The uninstallation secret is deleted after execution but before the node reset, which could cause issues if the reset fails and reconciliation needs to retry. Consider moving the secret deletion after the node reset.
Code suggestion
Check the AI-generated fix before applying
| if err := r.Client.Delete(ctx, secret); err != nil { | |
| logger.Error(err, "error deleting uninstallation secret") | |
| return err | |
| } | |
| } else { | |
| logger.Info("Skipping uninstallation of k8s components") | |
| } | |
| err := r.resetNode(ctx, byoHost) | |
| if err != nil { | |
| return err | |
| } | |
| } else { | |
| logger.Info("Skipping uninstallation of k8s components") | |
| } | |
| err := r.resetNode(ctx, byoHost) | |
| if err != nil { | |
| return err | |
| } | |
| if !r.SkipK8sInstallation && secret != nil { | |
| if err := r.Client.Delete(ctx, secret); err != nil { | |
| logger.Error(err, "error deleting uninstallation secret") | |
| return err | |
| } | |
| } | |
Code Review Run #d14744
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
2 participants
Made changes in the host_reconciler.go and reconciler_test.go to have the proper functionality and unit tests for the installation and uninstallation secret.
Summary by Bito
This PR enhances the host reconciler by fixing issues in installation and uninstallation secrets handling. It refines unit tests, modifies patch helper operations, and improves script parsing and execution logic while adding better logging and error reporting. The changes increase system robustness and provide clearer diagnostics during failures.