Skip to content

Conversation

@joelanford
Copy link
Member

Description of the change:

Add all supported resources to generated bundles and packagemanifests.

Motivation for the change:

SDK should automatically add all supported resources to generated bundles and packagemanifests.

Checklist

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

objs = append(objs, &c.ServiceAccounts[i])
}

// All Services passed in should be written.
Copy link
Member

Choose a reason for hiding this comment

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

This isn't quite true. OLM manages webhooks and their services, no explicitly defined webhook Service objects should be passed. You can filter out Services by finding those associated with {Mutating,Validating}WebhookConfigurations included in a bundled CSV (all webhook configs in the collector.Manifests).

Copy link
Member Author

Choose a reason for hiding this comment

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

Do we just match on namespace and name? What if a service declares multiple ports and one of them is for the webhook?

Do you think we need to filter out webhook service ports and then filter out any services that have no ports?

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 constrain the problem by filtering any service that matches a webhook config by name/namespace, and document that webhooks need their own separate service? Modifying a resource's functional content doesn't feel right.

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 could. I think I have a working solution that does what I suggest above. Let me know what you think.

@@ -0,0 +1,3 @@
entries:
- description: When generating bundles and packagemanifests, add all resources supported by OLM.
kind: change
Copy link
Member

Choose a reason for hiding this comment

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

I almost want to say this is a net-new feature because we explicitly do this for RBAC and no other types, but I am ok with "change" since the explicitness isn't user-facing.

@joelanford joelanford force-pushed the bundle-pm-supported-kinds branch from 2a9438c to be51cb5 Compare October 29, 2020 16:36
Copy link
Member

@estroz estroz 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 Oct 30, 2020
Copy link
Contributor

@camilamacedo86 camilamacedo86 left a comment

Choose a reason for hiding this comment

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

I have not a deep knowledge on this feature, however, the changes here shows all ok for me.

@camilamacedo86 camilamacedo86 added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 30, 2020
@joelanford joelanford merged commit 160151b into operator-framework:master Oct 30, 2020
@joelanford joelanford deleted the bundle-pm-supported-kinds branch October 30, 2020 18:25
reinvantveer pushed a commit to reinvantveer/operator-sdk that referenced this pull request Feb 5, 2021
…perator-framework#4137)

* generate/internal/manifests.go: add all supported kinds to manifests

* filter out webhook ports/services

* add all services to manifests

Signed-off-by: reinvantveer <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants