Skip to content

Conversation

alexandreLamarre
Copy link

These error wrappers and constants are probably not fully accurate or comprehensive, but this draft in intended to serve as a discussion of whether or not we can/should handle these directly in the SDK

Background

Some granular errors are surfaced through reg.Id as negative values.

This adds both constants and strongly-typed errors for downstream consumers to handle more easily these opaque error cases.

Motivation

Avoid patterns in downstream consumers of the client SDK like the rancher SCC operator, where we have to redefine these values as well as handle both the (int, error) responses and decide later which is an error case:
https://github.com/mallardduck/rancher/blob/b06c7c90fb76d04e6c715cb66ca1206f5b71c75c/pkg/scc/suseconnect/wrapper.go#L58-L83

Some granular errors are surfaced through reg.Id as negative values. This adds both constants and strongly-typed errors for downstream consumers to handle more easily these opaque error cases

Signed-off-by: Alexandre Lamarre <[email protected]>
return reg.Id, credErr
}
regErr := mapRegIdToErr(reg.Id)
return reg.Id, regErr
Copy link
Author

@alexandreLamarre alexandreLamarre Jun 9, 2025

Choose a reason for hiding this comment

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

Note that currently this a breaking change for consumers relying on handling reg.Id error cases based solely on the ID, which may or may not be important. It could be a good breaking change, in the sense that we may want to signal to end users that an error was actually encountered and the ID is not actually an ID

We can always omit returning the error here and consider some other options:

  • Only surface constants as part of improving error handling
  • Implement a public helper function like:
func ErrorFromRegistrationId(regId int) error {
  // same strongly-typed errors in the current control flow
}

@alexandreLamarre alexandreLamarre marked this pull request as ready for review June 10, 2025 13:01

if err := json.Unmarshal(response, &reg); err != nil {
return 0, err
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I have a digression here 😄

I understand that from a client perspective, as it stands now on errors you have to check whether error != nil, and also that reg.Id is valid. This can be cumbersome as it escapes Go's logic of "just check on the error type". Hence. Why not doing it after unmarshal but also keeping things more simple? At this point we know the value of reg.Id, and we could do something like:

	if reg.Id <= 0 {
		return reg.Id, errors.New("bad registration system ID") // or other error message, I don't care
	}

Then as a package we can provide the RegistrationSystemIdEmpty and other constants as you are suggesting, just in case the client wants to be more specific on the error. But things like mapRegIdToErr and similar should be a responsibility of the client, not this library.

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.

3 participants