Skip to content

build: support OwnNamespace installMode type#286

Merged
mergify[bot] merged 1 commit into
csi-addons:mainfrom
Rakshith-R:support-OwnNamespace
Jan 12, 2023
Merged

build: support OwnNamespace installMode type#286
mergify[bot] merged 1 commit into
csi-addons:mainfrom
Rakshith-R:support-OwnNamespace

Conversation

@Rakshith-R

Copy link
Copy Markdown
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
@mergify mergify Bot requested review from Yuggupta27, nixpanic and yati1998 January 3, 2023 11:49
@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>
@Rakshith-R Rakshith-R force-pushed the support-OwnNamespace branch from c118bed to 1f7b84f Compare January 11, 2023 05:40
installModes:
# CSVs featuring a conversion webhook can only support the AllNamespaces install mode
- supported: false
- supported: true

Copy link
Copy Markdown
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
Copy Markdown
Member

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Member Author

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

@Rakshith-R Rakshith-R requested a review from Madhu-1 January 11, 2023 05:45

@Madhu-1 Madhu-1 left a comment

Copy link
Copy Markdown
Member

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
Copy Markdown
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
black-dragon74 pushed a commit to black-dragon74/kubernetes-csi-addons that referenced this pull request Jul 31, 2025
…rry-pick-283-to-release-4.18

[release-4.18] DFBUGS-2301: fix: avoid panic due to incorrect logger key-value pairs
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