Skip to content

Conversation

@hjiawei
Copy link
Contributor

@hjiawei hjiawei commented May 30, 2025

Description

For PR author

  • Tests for change.
  • If changing pkg/apis/, run make gen-files
  • If changing versions, run make gen-versions

For PR reviewers

A note for code reviewers - all pull requests must have the following:

  • Milestone set according to targeted release.
  • Appropriate labels:
    • kind/bug if this is a bugfix.
    • kind/enhancement if this is a a new feature.
    • enterprise if this PR applies to Calico Enterprise only.

@Brian-McM
Copy link
Contributor

Brian-McM commented May 30, 2025

Why does the pointer library need to be changed? If you don't liked the typed functios, just use ptr.ToPtr (which I put in after generics came out).

@hjiawei
Copy link
Contributor Author

hjiawei commented May 30, 2025

There is nothing wrong with our implementation. An equivalent implementation already exists in the Kubernetes utility library we've imported. Leveraging the existing library helped reduce our codebase footprint, even if only slightly in this case.

@Brian-McM
Copy link
Contributor

Leveraging the existing library helped reduce our codebase footprint, even if only slightly in this case.

I personally don't agree with that, especially for small utilities. We're importing k8s for k8s utilities, not for general utils.

I've found this leads to using multiple different third parties to import basic utils and then if we stop using those utilities for the reasons we actually needed them, we're stuck with the dependencies / need a bigger refactor.

It makes it harder to maintain if there's even a slight descrepency between what we need and what they have, leading to our own implementations anyway.

In this case when it's just a single function it doesn't seem useful, especially when the function is already there (just unnecessary churn). Like I'm sure k8s and other libraries have implemented all the simple helper functions we have, I wouldn't advocate for swapping out all our small util functions for third party implementations across all our imports.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants