Skip to content

🐛 pkg/envtest: validate config when handling CRDs #3512

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.

Sign up for GitHub

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

Open
orangecms wants to merge 1 commit into
base: main
Choose a base branch
from
+16 −0
Open

🐛 pkg/envtest: validate config when handling CRDs #3512

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 16 additions & 0 deletions pkg/envtest/crd.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,10 @@ const (

// InstallCRDs installs a collection of CRDs into a cluster by reading the crd yaml files from a directory.
func InstallCRDs(config *rest.Config, options CRDInstallOptions) ([]*apiextensionsv1.CustomResourceDefinition, error) {
if config == nil {
Copy link
Copy Markdown
Member

@alvaroaleman alvaroaleman May 5, 2026

Choose a reason for hiding this comment

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

hi @orangecms, thanks for trying to help out but I am not in favor of merging this. Assuming that nil is a valid value for pointer arguments to a function or method when the arg is not explicitly documented as such is generally wrong and a programming mistake. Panicing is how the function signals this so the author can correct that mistake.

If you look at the go stdlib, you will find that nilchecking parameters is not commonly done.

/hold

Copy link
Copy Markdown
Author

@orangecms orangecms May 5, 2026

Choose a reason for hiding this comment

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

Hi, can you help me understand?

pkg/client does have that check, so it would be inconsistent to sometimes have the check, sometimes not.

Wouldn't the error returned be what allows for graceful handling and obtaining an understanding of the error? What would otherwise be the reason to have nil checks, such as in pkg/client?

Copy link
Copy Markdown
Member

@alvaroaleman alvaroaleman May 5, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

Yep, seems to be an oversight, the rest of the codebase doesn't have such checks and we don't plan on changing that.

return nil, fmt.Errorf("must provide non-nil rest.Config to InstallCRDs")
}

defaultCRDOptions(&options)

// Read the CRD yamls into options.CRDs
Expand Down Expand Up @@ -142,6 +146,10 @@ func defaultCRDOptions(o *CRDInstallOptions) {

// WaitForCRDs waits for the CRDs to appear in discovery.
func WaitForCRDs(config *rest.Config, crds []*apiextensionsv1.CustomResourceDefinition, options CRDInstallOptions) error {
if config == nil {
return fmt.Errorf("must provide non-nil rest.Config to WaitForCRDs")
}

// Add each CRD to a map of GroupVersion to Resource
waitingFor := map[schema.GroupVersion]*sets.Set[string]{}
for _, crd := range crds {
Expand Down Expand Up @@ -215,6 +223,10 @@ func (p *poller) poll(ctx context.Context) (done bool, err error) {

// UninstallCRDs uninstalls a collection of CRDs by reading the crd yaml files from a directory.
func UninstallCRDs(config *rest.Config, options CRDInstallOptions) error {
if config == nil {
return fmt.Errorf("must provide non-nil rest.Config to UninstallCRDs")
}

// Read the CRD yamls into options.CRDs
if err := ReadCRDFiles(&options); err != nil {
return err
Expand Down Expand Up @@ -242,6 +254,10 @@ func UninstallCRDs(config *rest.Config, options CRDInstallOptions) error {

// CreateCRDs creates the CRDs.
func CreateCRDs(config *rest.Config, crds []*apiextensionsv1.CustomResourceDefinition) error {
if config == nil {
return fmt.Errorf("must provide non-nil rest.Config to CreateCRDs")
}

cs, err := client.New(config, client.Options{})
if err != nil {
return fmt.Errorf("unable to create client: %w", err)
Expand Down