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.

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

build: support OwnNamespace installMode type #286

Merged
merged 1 commit into from
Jan 12, 2023

Conversation

Rakshith-R
Copy link
Member

We currently don't have a conversion webhook,
therefore can support the OwnNamespace installMode type too.
This change was done by default when webhook support was added.

Signed-off-by: Rakshith R rar@redhat.com

@Rakshith-R Rakshith-R added the DNM Do Not Merge label Jan 3, 2023
@Rakshith-R Rakshith-R marked this pull request as draft January 3, 2023 11:49
@Rakshith-R Rakshith-R force-pushed the support-OwnNamespace branch 2 times, most recently from 331ecb8 to c118bed Compare January 4, 2023 09:43
We currently don't have a conversion webhook,
therefore can support the OwnNamespace installMode
type too.
This change was done by default when webhook support
was added.

Signed-off-by: Rakshith R <rar@redhat.com>
@@ -52,8 +52,7 @@ spec:
deployments: null
strategy: ""
installModes:
# CSVs featuring a conversion webhook can only support the AllNamespaces install mode
- supported: false
- supported: true
Copy link
Member Author

Choose a reason for hiding this comment

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

since we don't have conversion for any CRDs, we can support OwnNamespace installmode.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Is is problematic for a deployment with OwnNamespace, and we introduce conversion webhooks later on? Can that deployment move to AllNamespaces?

Copy link
Member Author

Choose a reason for hiding this comment

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

Is is problematic for a deployment with OwnNamespace, and we introduce conversion webhooks later on? Can that deployment move to AllNamespaces?

That's the exact problem being faced right now.

OLM has no way to automatically upgrade operator group which influences installModes
https://olm.operatorframework.io/docs/advanced-tasks/operator-scoping-with-operatorgroups/

This operator group is installed by admin/UI, will need to manually changed/ discussed with olm devs.

But for now, we don't need to put this restriction since we don't have conversion webhooks right now.

Comment on lines +19 to +27
#- patches/webhook_in_volumereplicationclasses.yaml
#+kubebuilder:scaffold:crdkustomizewebhookpatch

# [CERTMANAGER] To enable cert-manager, uncomment all the sections with [CERTMANAGER] prefix.
# patches here are for enabling the CA injection for each CRD
#- patches/cainjection_in_csiaddonsnodes.yaml
#- patches/cainjection_in_reclaimspacejobs.yaml
#- patches/cainjection_in_volumereplications.yaml
- patches/cainjection_in_volumereplicationclasses.yaml
#- patches/cainjection_in_volumereplicationclasses.yaml
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 should be uncommitted for enabling webhook conversion,
since we don't have conversion for any CRDs, we don't need this.

@Rakshith-R Rakshith-R marked this pull request as ready for review January 11, 2023 05:45
@Rakshith-R
Copy link
Member Author

I've tested this pr and it works fine,
marking it ready for review.

Copy link
Member

@Madhu-1 Madhu-1 left a comment

Choose a reason for hiding this comment

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

Yes, this is good to go, To summarize, this is not a must for upstream, and no harm in keeping it also and this is a kind of temporary fix for downstream builds. If we add support for conversion webhooks in the future, the ODF should fix the way it's installing the CSI-addons operator right?

@Rakshith-R
Copy link
Member Author

Yes, this is good to go, To summarize, this is not a must for upstream, and no harm in keeping it also and this is a kind of temporary fix for downstream builds. If we add support for conversion webhooks in the future, the ODF should fix the way it's installing the CSI-addons operator right?

We don't need to have this restriction at the moment.
We can introduce it when we have conversion webhooks.
Other operators that depend on this must adapt accordingly at that time.

@Rakshith-R Rakshith-R removed the DNM Do Not Merge label Jan 11, 2023
@mergify mergify bot merged commit 65a7a08 into csi-addons:main Jan 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants