Skip to content
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

Update Kanister dependancy and API fixes #320

Merged
merged 4 commits into from
Jan 6, 2025
Merged

Conversation

PrasadG193
Copy link
Contributor

@PrasadG193 PrasadG193 commented Jan 3, 2025

This PR upgrades Kanister dependancy to 0abc731c8242a5af8682e46607f429563683da8e and fixes breaking APIs after depemdancy upgrade. With Kanister dep, the K8s dep also gets ugpraded to 1.31 which requires some refactor in the return statements. The fake K8s client no longer returns nil object in case of error, so the additional check for error is added.

Signed-off-by: Prasad Ghangal <[email protected]>
Signed-off-by: Prasad Ghangal <[email protected]>
@PrasadG193 PrasadG193 requested a review from bathina2 January 6, 2025 07:26
Signed-off-by: Prasad Ghangal <[email protected]>
@PrasadG193 PrasadG193 force-pushed the update-kanister-dep branch from 28f410b to 7e6e465 Compare January 6, 2025 18:16
@PrasadG193 PrasadG193 marked this pull request as ready for review January 6, 2025 18:17
Comment on lines +72 to +76
pvc, err := o.kubeCli.CoreV1().PersistentVolumeClaims(namespace).Get(ctx, pvcName, metav1.GetOptions{})
if err != nil {
return nil, err
}
return pvc, nil
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fake clients (in k8s 1.31 client-go sdk) no longer returns nil object in case of error. So we had to add explicit checks

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm shouldn't the caller be checking if error is nil before checking the value anyway? Or does this result in a lot of changes to UTs? Why can't we just return the objects as is?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alternatively, we'll have to refactor tests, instead of checking for nil, we'll have to check for empty structs. My intention was to have consistent values in the return. E.g in case of other types of error in the same function, we are returning nil, but in K8s API error, it returns empty struct. We'll also have to refactor UT accordingly to check for different value in case of different types of errors.

@PrasadG193 PrasadG193 changed the title Update Kanister dep and API fixes Update Kanister dependancy and API fixes Jan 6, 2025
@PrasadG193 PrasadG193 requested a review from pavannd1 January 6, 2025 18:21
@PrasadG193 PrasadG193 merged commit 08b1ab9 into master Jan 6, 2025
4 checks passed
@PrasadG193 PrasadG193 deleted the update-kanister-dep branch January 6, 2025 19:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants