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

Connector Ops CI Checks enforces that Beta+ connectors include an icon #19448

Closed
evantahler opened this issue Nov 15, 2022 · 4 comments
Closed

Comments

@evantahler
Copy link
Contributor

evantahler commented Nov 15, 2022

An icon should exist for Beta and GA connectors as part of the connector definitions and verify that it actually exists.

~~If possible, we can an also enforce the dimensions of the image (but timebox this work to ~1 day).~~

@sherifnada
Copy link
Contributor

@evantahler how does Connector Ops think about the dependency between SAT and the platform? asking since historically SAT has been very stand-alone (you just need a connector to test and no other information), and icons live in the platform so this is starting to introduce more of a platform dep

@evantahler
Copy link
Contributor Author

evantahler commented Nov 16, 2022

+1!

For this story specifically, I think that crossing the boundary is probably fine?

I think that connectors being stand-alone is a good goal, and long term we should probably make some changes. It's also pretty weird that there is information that the platform knows that a connector doesn't (UUID, release stage, etc). I think all of that information should be moved into the connector (maybe a yaml file, maybe docker tags) so that the source of truth for /all/ connector information. I imagine a connector-metadata.yaml file in every connector's directory which contains the information that's currently in source_definitions.yaml... and more information as we go. We then build connector-metadata.yaml from that.

@pedroslopez
Copy link
Contributor

pedroslopez commented Nov 16, 2022

I would push back on having SAT validate things in the definitions/platform - my view is that SAT should be focused on ensuring that a connector adheres to the airbyte protocol. I think certification requirements are related, but different from connector functionality. (test suite vs app store review checks)

I like how, for example, when saying that GA connectors need to pass certain additional tests as part of the SAT it became a more general "strictness level", and we have a separate CI check that does know more about things outside the connector and can verify that connectors of a given release stage use a certain strictness level in SAT. My proposal would be to keep building this up with more certification-specific checks, like icon existence and dimensions.

This would feel less of a problem if metadata was exposed as part of the protocol or by the connector somehow, kind of like @evantahler mentioned. As a side note our icon situation definitely needs revisiting since they're baked into the platform and we should remove anything connector-related that's baked in. It's also became a barrier for people using our current v0 CMS jsons because the icons just point to paths within the platform codebase so they're unusable, but that's a different topic 😛

@pedroslopez pedroslopez changed the title SAT enforces that Beta+ connectors include an icon Connector Ops CI Checks enforces that Beta+ connectors include an icon Nov 22, 2022
@alafanechere
Copy link
Contributor

This will be covered by #21715 as part of the "Deploy All Eligible connectors to cloud project" ( #20553 ) The /test command will check this, but it won't be part of SAT tests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants