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

normalization: delete supportsDbt and supportsNormalization from DestinationDefinitionSpecificationRead #21005

Merged
merged 23 commits into from
Jan 7, 2023
Merged

normalization: delete supportsDbt and supportsNormalization from DestinationDefinitionSpecificationRead #21005

merged 23 commits into from
Jan 7, 2023

Conversation

alafanechere
Copy link
Contributor

@alafanechere alafanechere commented Jan 4, 2023 •
edited
Loading

What

Closes #18239

Now that the source of truth for supportsDbt and normalizationConfig is in the destination definition and not in it's specification we need to remove these fields from the DestinationDefintionSpecificationRead model.

How

  1. Remove supportsDbt and supportsNormalization from DestinationDefintionSpecificationRead model
  2. Expose supportsDbt and normalizationConfigin DestinationDefinitionRead model as it is required by the front-end for the ConnectionTransformationTab.
  3. Update airbyte-webapp according to API changes
  4. Update octavia-cli according to API changes

Recommended reading order

  1. airbyte-api Open API configuration changes
  2. airbyte-server Handler logic update
  3. airbyte-webapp changes to reflect API updates
  4. octavia-cli changes to reflect API updates.

🚨 User Impact 🚨

No user impact is expected.

@octavia-squidington-iv octavia-squidington-iv added area/api Related to the api area/platform issues related to the platform labels Jan 4, 2023
@alafanechere alafanechere temporarily deployed to more-secrets January 4, 2023 08:52 — with GitHub Actions Inactive
@alafanechere alafanechere temporarily deployed to more-secrets January 4, 2023 08:53 — with GitHub Actions Inactive
@alafanechere alafanechere changed the title normalization: delete supportsDbt and supportsNormalization from DestinationDefinitionSpecificationRea normalization: delete supportsDbt and supportsNormalization from DestinationDefinitionSpecificationRead Jan 4, 2023
@octavia-squidington-iv octavia-squidington-iv added area/documentation Improvements or additions to documentation area/server area/frontend Related to the Airbyte webapp labels Jan 4, 2023
@alafanechere alafanechere temporarily deployed to more-secrets January 4, 2023 16:41 — with GitHub Actions Inactive
@alafanechere alafanechere temporarily deployed to more-secrets January 4, 2023 16:42 — with GitHub Actions Inactive
@alafanechere alafanechere marked this pull request as ready for review January 4, 2023 16:49
@alafanechere alafanechere requested a review from a team as a code owner January 4, 2023 16:49
@alafanechere alafanechere temporarily deployed to more-secrets January 4, 2023 16:50 — with GitHub Actions Inactive
@alafanechere alafanechere requested a review from a team January 4, 2023 16:50
@alafanechere alafanechere temporarily deployed to more-secrets January 4, 2023 16:50 — with GitHub Actions Inactive
@alafanechere
remove debug loggin
83ac433
@alafanechere
Merge branch 'augustin/normalization/clean-DestinationDefinitionSpeci…
08ee60d 
…ficationRead' of github.com:airbytehq/airbyte into augustin/normalization/clean-DestinationDefinitionSpecificationRead
@alafanechere alafanechere temporarily deployed to more-secrets January 4, 2023 17:29 — with GitHub Actions Inactive
@alafanechere alafanechere temporarily deployed to more-secrets January 4, 2023 17:29 — with GitHub Actions Inactive
evantahler
evantahler reviewed Jan 4, 2023
View reviewed changes
Copy link
Contributor

@evantahler evantahler left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Just comments for now, as the front-end is failing to build.

airbyte-api/src/main/openapi/config.yaml Show resolved Hide resolved
airbyte-server/src/main/java/io/airbyte/server/converters/ApiPojoConverters.java Outdated Show resolved Hide resolved
airbyte-server/src/main/java/io/airbyte/server/handlers/DestinationDefinitionsHandler.java Outdated Show resolved Hide resolved
...yte-webapp/src/pages/ConnectionPage/pages/ConnectionItemPage/ConnectionTransformationTab.tsx Show resolved Hide resolved
@alafanechere alafanechere temporarily deployed to more-secrets January 5, 2023 14:30 — with GitHub Actions Inactive
@alafanechere alafanechere temporarily deployed to more-secrets January 5, 2023 14:31 — with GitHub Actions Inactive
@evantahler evantahler temporarily deployed to more-secrets January 6, 2023 17:33 — with GitHub Actions Inactive
@alafanechere
Copy link
Contributor Author

@evantahler I already updated the branch and Front End E2E tests passed 👍 It's the Platform: Acceptance Tests that look flaky now...

@evantahler
Copy link
Contributor

@edmundito can we get a 👍 on the front-end now? We'll sort out the platform flakyness...

@alafanechere alafanechere disabled auto-merge January 6, 2023 17:39
edmundito
edmundito approved these changes Jan 6, 2023
View reviewed changes
Copy link
Contributor

@edmundito edmundito left a comment

Choose a reason for hiding this comment

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

FE approved

@evantahler evantahler enabled auto-merge (squash) January 6, 2023 17:55
@evantahler evantahler temporarily deployed to more-secrets January 6, 2023 18:42 — with GitHub Actions Inactive
@evantahler evantahler temporarily deployed to more-secrets January 6, 2023 18:42 — with GitHub Actions Inactive
@evantahler evantahler temporarily deployed to more-secrets January 6, 2023 22:10 — with GitHub Actions Inactive
@evantahler evantahler temporarily deployed to more-secrets January 6, 2023 22:11 — with GitHub Actions Inactive
@davinchia davinchia temporarily deployed to more-secrets January 7, 2023 00:37 — with GitHub Actions Inactive
@davinchia davinchia temporarily deployed to more-secrets January 7, 2023 00:38 — with GitHub Actions Inactive
@evantahler evantahler merged commit e4707f5 into master Jan 7, 2023
@evantahler evantahler deleted the augustin/normalization/clean-DestinationDefinitionSpecificationRead branch January 7, 2023 01:12
@github-actions github-actions bot mentioned this pull request Jan 9, 2023
Closed
@evantahler evantahler mentioned this pull request Jan 10, 2023
Closed
37 tasks
@evantahler
Copy link
Contributor

evantahler commented Jan 10, 2023 •
edited
Loading

@alafanechere comparing this PR to #20854, there seem to be a lot more deletions/cleanup we can do in spec.json files. I would also expect to see it removed from the Airbyte protocol?

airbyte/airbyte-protocol/protocol-models/src/main/resources/airbyte_protocol/airbyte_protocol.yaml

Line 457 in 510a303

supportsNormalization:

@alafanechere alafanechere mentioned this pull request Jan 12, 2023
Closed
@octavia-squidington-iii octavia-squidington-iii mentioned this pull request Jan 12, 2023
Merged
jbfbell pushed a commit that referenced this pull request Jan 13, 2023
@alafanechere @evantahler @jbfbell
normalization: delete supportsDbt and supportsNormalization from Dest…
972794a 
…inationDefinitionSpecificationRead (#21005)

* delete supportsDbt and supportsNormalization from DestinationDefinitionSpecificationRead

* FE changes

* add new fields to the DestinationDefinitionRead API model

* update handlers

* update api doc

* update handlers

* remove debug loggin

* implement suggestions

* update octavia-cli tests

* fix supported check

* update octavia-cli

* fix integration tests

* fix mocks

* fix forgotten renaming

Co-authored-by: Evan Tahler <evan@airbyte.io>
@joel-olazagasti joel-olazagasti mentioned this pull request Apr 11, 2023
Closed
nina-j added a commit to nina-j/dagster that referenced this pull request May 9, 2023
@nina-j
fix: Add new normalization config check
3cce17c 
Fixes dagster-io#13524.
The Airbyte API changed source of truth for normalization availability in airbytehq/airbyte#21005.
This commit adds an additional check for normalization configuration in
`does_dest_support_normalization`. The current check is kept to
ensure backward compatibility - thus, if any of the checks are true,
the function returns true.
@nina-j nina-j mentioned this pull request May 9, 2023
Merged
nina-j added a commit to nina-j/dagster that referenced this pull request May 9, 2023
@nina-j
fix(dagster_airbyte): Add new normalization config check
638a11d 
Fixes dagster-io#13524.
The Airbyte API changed source of truth for normalization availability in airbytehq/airbyte#21005.
This commit adds an additional check for normalization configuration in
`does_dest_support_normalization`. The current check is kept to
ensure backward compatibility - thus, if any of the checks are true,
the function returns true.
smackesey pushed a commit to dagster-io/dagster that referenced this pull request May 9, 2023
@nina-j
fix(dagster_airbyte): Add new normalization config check (#14174)
fc65f63 
## Summary & Motivation
The Airbyte API changed source of truth for normalization availability
in airbytehq/airbyte#21005. This commit adds an
additional check for normalization configuration in
`does_dest_support_normalization`. The current check is kept to ensure
backward compatibility - thus, if any of the checks are true, the
function returns true.

Fixes #13524.

## How I Tested These Changes
I ran the test suite for the `dagster_airbyte` module and added tests
that cover my changes.

Co-authored-by: Nina Jensen <nije@boozt.com>
salazarm pushed a commit to dagster-io/dagster that referenced this pull request May 10, 2023
@nina-j @salazarm
fix(dagster_airbyte): Add new normalization config check (#14174)
42fa93a 
## Summary & Motivation
The Airbyte API changed source of truth for normalization availability
in airbytehq/airbyte#21005. This commit adds an
additional check for normalization configuration in
`does_dest_support_normalization`. The current check is kept to ensure
backward compatibility - thus, if any of the checks are true, the
function returns true.

Fixes #13524.

## How I Tested These Changes
I ran the test suite for the `dagster_airbyte` module and added tests
that cover my changes.

Co-authored-by: Nina Jensen <nije@boozt.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@evantahler evantahler evantahler approved these changes

@edmundito edmundito edmundito approved these changes

Assignees
No one assigned
Labels
area/api Related to the api area/documentation Improvements or additions to documentation area/frontend Related to the Airbyte webapp area/platform issues related to the platform area/server
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Remove supportsDbt and supportsNormalization from DestinationDefinitionSpecificationRead
5 participants
@alafanechere @edmundito @evantahler @davinchia @octavia-squidington-iv