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

Manage builtin-connectors as CRD #585

Merged
merged 21 commits into from
Mar 8, 2023
Merged

Manage builtin-connectors as CRD #585

merged 21 commits into from
Mar 8, 2023

Conversation

alpreu
Copy link
Contributor

@alpreu alpreu commented Feb 27, 2023 •
edited
Loading

Master Issue: #566

Motivation

We want to start managing the available connectors in Function Mesh by providing a cluster-scoped CRD where users can provide their connector definitions. Different than the existing CRDs, the ConnectorCatalog CRD is only used to store connector metadata, there is no operator that will create pods etc.

Modifications

Added a ConnectorCatalog CRD and webhook.

Verifying this change

This change added tests and can be verified as follows:

  • Added a sample yaml that to test deploying CRs into e.g. kind clusters

Documentation

Check the box below.

Need to update docs?

  • doc-required

    (If you need help on updating docs, create a doc issue)

  • no-need-doc

    (Please explain why)

  • doc

    (If this PR contains doc changes)

I believe this change (and the related ones in function-mesh-worker-service) will require updating the documentation on the function-mesh website.

Alexander Preuß added 2 commits February 27, 2023 12:58
@github-actions
Copy link

@alpreu:Thanks for your contribution. For this PR, do we need to update docs?
(The PR template contains info about doc, which helps others know more about the changes. Can you provide doc-related info in this and future PR descriptions? Thanks)

@github-actions github-actions bot added the doc-info-missing This pr needs to mark a document option in description label Feb 27, 2023
@alpreu alpreu requested review from tpiperatgod and danpi February 28, 2023 15:05
@alpreu
Copy link
Contributor Author

alpreu commented Feb 28, 2023

Please check if there is anything missing in this PR, I'm not that familiar with this codebase yet

@alpreu alpreu marked this pull request as ready for review March 1, 2023 14:45
@alpreu alpreu requested review from nlu90, freeznet and a team as code owners March 1, 2023 14:45
@github-actions github-actions bot removed the doc-info-missing This pr needs to mark a document option in description label Mar 1, 2023
@github-actions
Copy link

github-actions bot commented Mar 1, 2023

@alpreu:Thanks for providing doc info!

@github-actions github-actions bot added the doc-required This pr needs a document label Mar 1, 2023
nlu90
nlu90 previously approved these changes Mar 2, 2023
View reviewed changes
Copy link
Contributor

@nlu90 nlu90 left a comment

Choose a reason for hiding this comment

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

Please resolve the failed unit test.

@freeznet, Could you also help take a look?

@Huanli-Meng Huanli-Meng mentioned this pull request Mar 3, 2023
Closed
8 tasks
tpiperatgod
tpiperatgod previously approved these changes Mar 3, 2023
View reviewed changes
Copy link
Contributor

@tpiperatgod tpiperatgod left a comment

Choose a reason for hiding this comment

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

lgtm

freeznet
freeznet previously approved these changes Mar 6, 2023
View reviewed changes
Copy link
Member

@freeznet freeznet left a comment

Choose a reason for hiding this comment

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

lgtm, but could you please provide a complete example of ConfigFieldDefinition in this PR?
Besides that, will this PR introduces the ClusterConnectorCatalog CRD or it will introduced in another PR? thanks.

@alpreu
Copy link
Contributor Author

alpreu commented Mar 6, 2023

lgtm, but could you please provide a complete example of ConfigFieldDefinition in this PR? Besides that, will this PR introduces the ClusterConnectorCatalog CRD or it will introduced in another PR? thanks.

@freeznet Yes, I will modify the sample.

Regarding the ClusterConnectorCatalog the current CRD is already cluster-scoped and I think we don't need to introduce a namespace-level CRD. Submitting custom connectors is still possible through either package upload or by passing an archive. I feel that adding another CRD adds little benefit but complicates everything. What do you think?

@alpreu alpreu dismissed stale reviews from freeznet, tpiperatgod, and nlu90 via 364cc8d March 7, 2023 11:24
@alpreu alpreu merged commit 51e480f into master Mar 8, 2023
@alpreu alpreu deleted the connector-catalog-crd branch March 8, 2023 14:23
@alpreu alpreu mentioned this pull request Mar 13, 2023
Closed
@Huanli-Meng Huanli-Meng mentioned this pull request Mar 20, 2023
Closed
freeznet added a commit that referenced this pull request Aug 3, 2023
@freeznet
Revert "Manage builtin-connectors through ConnectorCatalog CRD (#585)"
364f383 
This reverts commit 51e480f.

# Conflicts:
#	go.mod
#	main.go
#	pkg/webhook/connectorcatalog_webhook.go
nlu90 pushed a commit that referenced this pull request Aug 9, 2023
@freeznet
remove connector catalog CRD (#672)
f6dc2e2 
* Revert "Replace ConnectorCatalog.ConfigFieldDefinitions with SourceConfigFieldDefinitions and SinkConfigFieldDefinitions (#610)"

This reverts commit 77d77e9.

# Conflicts:
#	charts/function-mesh-operator/charts/admission-webhook/templates/crd-compute.functionmesh.io-connectorcatalogs.yaml

* Revert "Manage builtin-connectors through ConnectorCatalog CRD (#585)"

This reverts commit 51e480f.

# Conflicts:
#	go.mod
#	main.go
#	pkg/webhook/connectorcatalog_webhook.go

* revert changes

* revert main.go

* revert common.go
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tpiperatgod tpiperatgod tpiperatgod approved these changes

@nlu90 nlu90 nlu90 left review comments

@freeznet freeznet freeznet left review comments

@danpi danpi Awaiting requested review from danpi

Assignees

@alpreu alpreu

Labels
doc-required This pr needs a document m/2023-03
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@alpreu @freeznet @tpiperatgod @nlu90