Skip to content

✨ Update fields in the spec to no longer be a pointer #1171

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

Closed
rashmigottipati wants to merge 2 commits into from
Closed

✨ Update fields in the spec to no longer be a pointer #1171

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
32373a7
Update fields in the spec to no longer be a pointer
rashmigottipati Aug 22, 2024
25c5f55
add omitempty tag to optional fields
rashmigottipati Aug 22, 2024
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
6 changes: 3 additions & 3 deletions api/v1alpha1/clusterextension_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -265,7 +265,7 @@ type ClusterExtensionSpec struct {
// When not specified, the default configuration for each preflight check will be used.
//
//+optional
Preflight *PreflightConfig `json:"preflight,omitempty"`
Preflight PreflightConfig `json:"preflight,omitempty"`

// serviceAccount is a required reference to a ServiceAccount that exists
// in the installNamespace. The provided ServiceAccount is used to install and
Expand Down Expand Up @@ -319,7 +319,7 @@ type PreflightConfig struct {
// consequences of upgrading a CRD, such as data loss.
//
// This field is required if the spec.preflight field is specified.
CRDUpgradeSafety *CRDUpgradeSafetyPreflightConfig `json:"crdUpgradeSafety"`
CRDUpgradeSafety CRDUpgradeSafetyPreflightConfig `json:"crdUpgradeSafety,omitempty"`
Copy link
Contributor

@everettraven everettraven Aug 26, 2024

Choose a reason for hiding this comment

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

This field is a required field and therefore should not have the omitempty in the JSON tag:

Suggested change
CRDUpgradeSafety CRDUpgradeSafetyPreflightConfig `json:"crdUpgradeSafety,omitempty"`
CRDUpgradeSafety CRDUpgradeSafetyPreflightConfig `json:"crdUpgradeSafety"`

Copy link
Member Author

@rashmigottipati rashmigottipati Aug 26, 2024

Choose a reason for hiding this comment

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

I originally left it without the omitempty tag, however, unit tests are failing with the below error i.e. the field having a null value when it is empty:
Error Trace: /Users/rashmigottipati/go/src/github.com/operator-framework/operator-controller/internal/controllers/clusterextension_controller_test.go:63 Error: Received unexpected error: ClusterExtension.olm.operatorframework.io "cluster-extension-test-s58x6kfr" is invalid: [spec.preflight.crdUpgradeSafety.policy: Unsupported value: "": supported values: "Enabled", "Disabled", <nil>: Invalid value: "null": some validation rules were not checked because the object was invalid; correct the existing errors to complete validation]

Since these fields are required only if the spec.preflight field is specified, there's a possibility of it being null when the spec.preflight field is not being set and so I think it would be ok to add the omitempty tag to this field and also the Policy field but open to suggestions on how to appropriately handle this in a better way.

Copy link
Contributor

@everettraven everettraven Aug 26, 2024

Choose a reason for hiding this comment

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

Is there a way we could ensure the default value is set when the top level spec.preflight field is not explicitly set?

Copy link
Contributor

@everettraven everettraven Aug 26, 2024

Choose a reason for hiding this comment

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

if not, we may need to have some more complex CEL validations in place. We need to be careful there as well though to ensure the cost of the CEL expression is not too expensive.

Copy link
Contributor

@everettraven everettraven Aug 26, 2024

Choose a reason for hiding this comment

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

I found kubernetes-sigs/controller-tools#622 (comment) which may be what we need to do in this case.

Copy link
Member

@joelanford joelanford Aug 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Keep in mind. This is required for now, only because there are no other sibling preflight check configurations. As soon as there are sibling configurations (e.g. for a permissions preflight check), then we'll need a validation like "at least one of the preflight check fields is required".

With that in mind, perhaps it is reasonable to go ahead and implement a CEL expression based validation that checks "at least one of" semantics, where the check starts out as at least one of: [crdUpgradeSafety]

Copy link
Contributor

@everettraven everettraven Aug 26, 2024

Choose a reason for hiding this comment

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

Thats a good point @joelanford . I do think it makes sense to go ahead and update it such that we have the "at least one of" approach

}

// CRDUpgradeSafetyPreflightConfig is the configuration for CRD upgrade safety preflight check.
Expand All @@ -340,7 +340,7 @@ type CRDUpgradeSafetyPreflightConfig struct {
//
//+kubebuilder:validation:Enum:="Enabled";"Disabled"
//+kubebuilder:default:=Enabled
Policy CRDUpgradeSafetyPolicy `json:"policy"`
Policy CRDUpgradeSafetyPolicy `json:"policy,omitempty"`
Copy link
Contributor

@everettraven everettraven Aug 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

This is a required field and therefore should not have the omitempty JSON tag:

Suggested change
Policy CRDUpgradeSafetyPolicy `json:"policy,omitempty"`
Policy CRDUpgradeSafetyPolicy `json:"policy"`

}

const (
Expand Down
12 changes: 2 additions & 10 deletions api/v1alpha1/zz_generated.deepcopy.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 0 additions & 4 deletions config/base/crd/bases/olm.operatorframework.io_clusterextensions.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -253,11 +253,7 @@ spec:
- Enabled
- Disabled
type: string
required:
- policy
type: object
required:
- crdUpgradeSafety
type: object
serviceAccount:
description: |-
Expand Down
10 changes: 4 additions & 6 deletions internal/applier/helm.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,12 +69,10 @@ func (h *Helm) Apply(ctx context.Context, contentFS fs.FS, ext *ocv1alpha1.Clust
}

for _, preflight := range h.Preflights {
if ext.Spec.Preflight != nil && ext.Spec.Preflight.CRDUpgradeSafety != nil {
if _, ok := preflight.(*crdupgradesafety.Preflight); ok && ext.Spec.Preflight.CRDUpgradeSafety.Policy == ocv1alpha1.CRDUpgradeSafetyPolicyDisabled {
// Skip this preflight check because it is of type *crdupgradesafety.Preflight and the CRD Upgrade Safety
// preflight check has been disabled
continue
}
if _, ok := preflight.(*crdupgradesafety.Preflight); ok && ext.Spec.Preflight.CRDUpgradeSafety.Policy == ocv1alpha1.CRDUpgradeSafetyPolicyDisabled {
// Skip this preflight check because it is of type *crdupgradesafety.Preflight and the CRD Upgrade Safety
// preflight check has been disabled
continue
}
switch state {
case StateNeedsInstall:
Expand Down