Move error variables out of utils package

[killianmuldoon]

Detailed Description

There are a few errors defined in the top level utils package that are better placed in the top level errors package. These errors should be deprecated and recreated in the errors package. The util errors should then be removed in a coming release.

Examples:

cluster-api/util/util.go

Line 58 in 573f005

ErrNoCluster = fmt.Errorf("no %q label present", clusterv1.ClusterLabelName)

cluster-api/util/util.go

Line 62 in 573f005

ErrUnstructuredFieldNotFound = fmt.Errorf("field not found")

/kind cleanup

[killianmuldoon]

/kind cleanup

[vincepri]

These seems only used or returned within that package, should we consider instead moving the functions using them to a different package instead?

[vincepri]

/milestone Next

[killianmuldoon]

These do seem to be used - ErrNoCluster in kubeadmconfig_controller.go and ErrUnstructuredFieldNotFound in cluster_controller_phases.go. If they were just used in package I'd say keep them, but it looks like they're being used as importable error messages which says to me they belong in errors.

[vincepri]

Are they used for error checking outside of util?

[killianmuldoon]

cluster-api/bootstrap/kubeadm/controllers/kubeadmconfig_controller.go

Line 166 in 58905fb

if errors.Cause(err) == util.ErrNoCluster {

cluster-api/controllers/cluster_controller_phases.go

Line 191 in 58905fb

if err := util.UnstructuredUnmarshalField(infraConfig, &cluster.Status.FailureDomains, "status", "failureDomains"); err != nil && err != util.ErrUnstructuredFieldNotFound {

[DiptoChakrabarty]

hey can I try this issue out , I am new here , this is my approach
move these errors in util package

cluster-api/util/util.go

Lines 56 to 62 in 573f005

	// ErrNoCluster is returned when the cluster
	// label could not be found on the object passed in.
	ErrNoCluster = fmt.Errorf("no %q label present", clusterv1.ClusterLabelName)
	// ErrUnstructuredFieldNotFound determines that a field
	// in an unstructured object could not be found.
	ErrUnstructuredFieldNotFound = fmt.Errorf("field not found")

to the error package here -> https://github.com/kubernetes-sigs/cluster-api/blob/58905fb5140a7844c3e24aa70d255fb8f9d0c491/errors/consts.go

then replace the util.ErrNoCluster and similar errors with the errors package ones

does this sound appropriate

[killianmuldoon]

@DiptoChakrabarty That sounds right - it's important to not remove the original variables from the utils package and instead add a deprecation message so we can remove the errors with the next breaking changes to Cluster API. So:

1. Add a deprecation message to the errors we'd like to move. (example of an existing deprecation message:

   cluster-api/api/v1alpha3/common_types.go

   Line 134 in 58905fb

   // Deprecated: This field has no function and is going to be removed in a next release.

2. Add new errors (with an appropriate error type) to the errors package.

3. Replace the errors as used in the rest of the code with the new version from the errors package.

[DiptoChakrabarty]

Hey I have done just like that with a bit of changes since importing the clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1" causes an import cycle

and I noticed that the error names are defined here https://github.com/kubernetes-sigs/cluster-api/blob/main/errors/consts.go

and the errors are configured here https://github.com/kubernetes-sigs/cluster-api/blob/main/errors/clusters.go so I made particular functions for these two util errors

[killianmuldoon]

#5419 (comment)

There's a couple of problems with this approach as it looks out of the scope of the current errors package. Currently the errors package covers mostly status conditions whereas these are closer to client errors.

Should we expand the overall usage of the errors package to include client errors like ClusterNotFound? These are commonly used and compared errors across the code base, but there's unwanted constraints we could possibly introduce by trying to implement them all in one format.

[k8s-triage-robot]

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

• Mark this issue or PR as fresh with /remove-lifecycle stale
• Mark this issue or PR as rotten with /lifecycle rotten
• Close this issue or PR with /close
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

[vincepri]

@killianmuldoon @DiptoChakrabarty Is it ok to close this issue and the related PR based on #5419 (comment)?

[killianmuldoon]

I'm fine with that given there's no agreed way to move those errors right now.

[vincepri]

👍
/close

[k8s-ci-robot]

@vincepri: Closing this issue.

In response to this:

👍
/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.