Skip to content
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

Jump to bottom

Make subscription spec.config and spec.config.resources optional #97

Merged
merged 1 commit into from
Feb 22, 2021
Merged

Make subscription spec.config and spec.config.resources optional #97

merged 1 commit into from
Feb 22, 2021

Conversation

joelanford
Copy link
Member

@joelanford
pkg/operators/v1alpha1/subscription_types.go: make config and resourc…
ceccb7f 
…es structs optional via pointer

Signed-off-by: Joe Lanford <joe.lanford@gmail.com>
@joelanford
Copy link
Member Author

This is a breaking change for Go users of operator-framework/api.

As far as I can tell, this is NOT a breaking change from a Kubernetes API standpoint. The current CRD does not require spec.config or spec.config.resources (which has no required fields).

@joelanford joelanford changed the title Subscription make spec.config and spec.config.resources optional Make subscription spec.config and spec.config.resources optional Feb 22, 2021
@joelanford
Copy link
Member Author

There's likely a way to fix this on the OLM side without making an API change by treating an empty struct identically to a nil struct pointer. However that has the downside that it becomes impossible to use spec.config.resources to unset resources defined by the CSV deployment.

joelanford
joelanford commented Feb 22, 2021
View reviewed changes
pkg/operators/v1alpha1/subscription_types.go
Channel string `json:"channel,omitempty"`
StartingCSV string `json:"startingCSV,omitempty"`
InstallPlanApproval Approval `json:"installPlanApproval,omitempty"`
Config *SubscriptionConfig `json:"config,omitempty"`
Copy link
Member Author

Choose a reason for hiding this comment

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

This change is not directly related to bug fix with overriding resources. However it seems that the intent is for spec.config to be optional, so it seems like a reasonable time to make this change as well.

dinhxuanvu
dinhxuanvu approved these changes Feb 22, 2021
View reviewed changes
Copy link
Member

@dinhxuanvu dinhxuanvu left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 22, 2021
@joelanford joelanford merged commit 718d279 into operator-framework:master Feb 22, 2021
@joelanford joelanford deleted the fix/dep-resources branch February 22, 2021 17:43
@njhale
Copy link
Member

njhale commented Feb 22, 2021

/approve

@hasbro17 hasbro17 mentioned this pull request Mar 4, 2021
Closed
5 tasks
@dependabot dependabot bot mentioned this pull request Mar 15, 2021
Merged
@joelanford joelanford mentioned this pull request Apr 6, 2021
Merged
@joelanford joelanford mentioned this pull request Apr 13, 2021
Closed
@ebaron ebaron mentioned this pull request Jul 6, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dinhxuanvu dinhxuanvu dinhxuanvu approved these changes

@awgreene awgreene Awaiting requested review from awgreene

@njhale njhale Awaiting requested review from njhale

Assignees

@dinhxuanvu dinhxuanvu

Labels
lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@joelanford @njhale @dinhxuanvu @openshift-ci-robot