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

generate: default CSV admissionReviewVersions and sideEffects #3903

Merged
merged 1 commit into from
Sep 18, 2020
Merged

generate: default CSV admissionReviewVersions and sideEffects #3903

merged 1 commit into from
Sep 18, 2020

Conversation

estroz
Copy link
Member

@estroz estroz commented Sep 17, 2020

Description of the change:

  • internal/generate/clusterserviceversion: default admissionReviewVersions to "v1beta1" and sideEffects to "None"

Motivation for the change: a CSV is not valid if its webhook definitions do not contain admissionReviewVersions or sideEffects, each of which can be defaulted with reasonable values.

/kind feature

Checklist

If the pull request includes user-facing changes, extra documentation is required:

@openshift-ci-robot openshift-ci-robot added the kind/feature Categorizes issue or PR as related to a new feature. label Sep 17, 2020
@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 17, 2020
@estroz estroz force-pushed the feature/csv-admissionreviewversions branch from a8ec8f1 to 65939a4 Compare September 17, 2020 16:45
@openshift-ci-robot openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 17, 2020
@estroz estroz force-pushed the feature/csv-admissionreviewversions branch from 65939a4 to 1e8275a Compare September 17, 2020 16:47
@estroz
Copy link
Member Author

estroz commented Sep 17, 2020

/cc @varshaprasad96

varshaprasad96
varshaprasad96 requested changes Sep 17, 2020
View reviewed changes
internal/generate/clusterserviceversion/clusterserviceversion_updaters.go
if description.SideEffects == nil {
seNone := admissionregv1.SideEffectClassNone
description.SideEffects = &seNone
}

if serviceRef := webhook.ClientConfig.Service; serviceRef != nil {
if serviceRef.Port != nil {
Copy link
Member

Choose a reason for hiding this comment

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

Can we include containerPort too?
The reason being this.

Copy link
Member Author

Choose a reason for hiding this comment

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

We shouldn't actually be defaulting this value, since a ServiceReference already does to 443. If OLM or a validator requires this value be set, that should change instead of the SDK potentially incorrectly defaulting this value. @awgreene @kevinrizza.

Copy link
Member

Choose a reason for hiding this comment

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

ContainerPort is not a required field and OLM defaults to 443 if the value is not set.

varshaprasad96
varshaprasad96 approved these changes Sep 18, 2020
View reviewed changes
Copy link
Member

@varshaprasad96 varshaprasad96 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 Sep 18, 2020
@estroz estroz merged commit 6f4dae4 into operator-framework:master Sep 18, 2020
@estroz estroz deleted the feature/csv-admissionreviewversions branch September 18, 2020 18:05
@awgreene awgreene mentioned this pull request Oct 8, 2020
Closed
@estroz
Copy link
Member Author

estroz commented Oct 9, 2020

This is actually a bugfix, not a feature, since a CSV with webhook definitions lacking these values is invalid.

/remove-kind feature
/kind bug

@openshift-ci-robot openshift-ci-robot added kind/bug Categorizes issue or PR as related to a bug. and removed kind/feature Categorizes issue or PR as related to a new feature. labels Oct 9, 2020
@estroz estroz mentioned this pull request Oct 9, 2020
Merged
2 tasks
@estroz
Copy link
Member Author

estroz commented Oct 9, 2020

/cherry-pick v1.0.x

@openshift-cherrypick-robot openshift-cherrypick-robot mentioned this pull request Oct 9, 2020
Closed
@openshift-cherrypick-robot
Copy link

@estroz: new pull request created: #4006

In response to this:

/cherry-pick v1.0.x

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.

estroz pushed a commit to estroz/operator-sdk that referenced this pull request Oct 12, 2020
@estroz
generate: default CSV admissionReviewVersions and sideEffects (o…
e852b7d 
…perator-framework#3903)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@awgreene awgreene awgreene left review comments

@varshaprasad96 varshaprasad96 varshaprasad96 approved these changes

@camilamacedo86 camilamacedo86 Awaiting requested review from camilamacedo86

@theishshah theishshah Awaiting requested review from theishshah

Assignees

@varshaprasad96 varshaprasad96

Labels
kind/bug Categorizes issue or PR as related to a bug. 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.
5 participants
@estroz @openshift-cherrypick-robot @awgreene @varshaprasad96 @openshift-ci-robot