Migrations for scoped connectors #11305
Conversation
to avoid having conditionals in every method with the tombstone boolean
| private Condition includeTombstones(final Field<Boolean> tombstoneField, final boolean includeTombstones) { | ||
| if (includeTombstones) { | ||
| return DSL.trueCondition(); | ||
| } else { | ||
| return tombstoneField.eq(false); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
this one doesn't fit in this PR thematically but I wanted to use this convenience method in multiple upcoming PRs
| description: true if this connector definition is available to all workspaces | ||
| type: boolean | ||
| default: false | ||
| custom: |
There was a problem hiding this comment.
from @michel-tricot in https://github.com/airbytehq/airbyte/pull/11339/files#r832831845
What is the definition of custom vs public? I would be careful before adding that kind of specific flags on the data model. They risk proliferating.
Are they mutually exclusive? then it should be a enum and if we foresee that it is only these two states then just one boolean
Can it be true on both? false on both? what is the expected behavior in these cases?
There was a problem hiding this comment.
Here's my take on what the possible combinations mean:
Connector Definition
| public | custom | Valid? | Meaning |
|---|---|---|---|
| true | true | no | |
| true | false | yes | Part of Airbyte’s catalog, available by default |
| false | true | yes | User-uploaded custom connector |
| false | false | yes | Part of Airbyte’s catalog, opt-in via workspace grant |
Originally, the plan was just to introduce the public boolean on its own. But, we want to support an endpoint that returns all public: false connector definitions that are eligible for opt-in grants. The use case here is an 'early access' connector that is in the Airbyte Catalog, but not available by default.
We need to clearly differentiate these private, opt-in connectors from user-uploaded custom connectors so that an instance admin doesn't accidentally grant a custom connector to a different customer. The data model doesn't currently support this differentiation, so we decided that adding a 'custom' boolean in addition to the 'public' boolean made sense.
There was a problem hiding this comment.
Yup. It also seems possible that public: true && custom: true could map to something valid in the future (for example third-party supported and maintained connectors that are approved by Airbyte as available to everyone).
I do agree though that we should be wary of flag proliferation and the current states can be conveniently captured in a single enum as well.
fyi the original discussion about whether to use an enum for this is here
There was a problem hiding this comment.
@cgardens fyi in case you want to weigh in, since you were part of the original discussion about boolean vs enum tradeoffs.
I think the original resistance to an enum came from the fact that we'd be duplicating information that we can already glean from the source_type enum, which already has a 'custom' value.
However, this source_type enum wouldn't be appropriate for describing destination definitions, and existing use-cases of source_type: custom would IMO be better served by querying for a new custom: true boolean.
In general, I think conflating custom with public visibility at the database level by trying to define an enum that describes both feels a bit inflexible to me. I think there are independent use-cases of each, so defining two booleans feels like the cleanest approach for supporting current needs while remaining flexible to the future.
There was a problem hiding this comment.
Agreed with @pmossman 's comment. The original source_type enum (originated for billing purposes) was misleading. e.g. we had: api, file, db, custom in a single enum. One of these things is not like the others. Custom is a different concept from other enums and concepts we've discussed and having it be clearly separate makes that clear. Private / public are also totally disjoint concepts from the custom idea. Parker's table lays it out well (and we should probably integrate it into how we document the project).
airbyte-config/init/src/test/java/io/airbyte/config/init/YamlSeedConfigPersistenceTest.java
Show resolved
Hide resolved
...java/io/airbyte/db/instance/configs/migrations/V0_35_59_001__AddPublicToActorDefinition.java
Outdated
Show resolved
Hide resolved
…ed-connectors-migrations # Conflicts: # airbyte-config/persistence/src/main/java/io/airbyte/config/persistence/ConfigRepository.java
What
Part of #9652
Tech Spec
Migrations and model changes to support scoped connector definitions.
Also includes some misc. changes that will be used in upcoming PRs.
Changes
public: Iftruethis definition can be used by all workspaces with no further permissions required. Otherwise, a workspace needs to have a grant for this definition in order to be able to use it.custom: iftruethis definition should only be configured for the workspace it was created in and should not be grantable to other namespacesactor_definition_workspace_grantsRecommended reading order
Easiest to view by commit
User Impact
No user visible impact. Upcoming PRs will depend on these changes.