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

ensure workspace webhook configs can be persisted correctly #18034

Merged
merged 4 commits into from
Oct 16, 2022
Merged

ensure workspace webhook configs can be persisted correctly #18034

merged 4 commits into from
Oct 16, 2022

Conversation

mfsiega-airbyte
Copy link
Contributor

What

Makes sure that the workspace-level webhook operation configs are passed correctly between API and persistence.

How

  • Include webhook configs in the workspace/update route.
  • Make sure that the database layer properly handles them.
  • Write some custom JSON handling to read the webhook configs. This is necessary because the only way -- currently -- to get the stored JSON blob to properly deserialise is to first hydrate the secrets (since the secrets were split out before writing the JSON blob), and then deserialise. In other words, the JSON that's in the database doesn't conform to the schema because of the secrets. Doing this custom handling lets us get the readonly bits of the config without having to unnecessarily hydrate the secrets (including a round trip to the secrets manager, and increasing some exposure risk).

Recommended reading order

  1. airbyte-api/src/main/openapi/config.yaml
  2. airbyte-server/src/main/java/io/airbyte/server/handlers/WorkspacesHandler.java
  3. airbyte-server/src/main/java/io/airbyte/server/converters/WorkspaceWebhookConfigsConverter.java

Testing

Validated e2e that webhook configs can be written, updated, and read. Added some unit test coverage.

@github-actions github-actions bot added area/api Related to the api area/documentation Improvements or additions to documentation area/platform issues related to the platform area/server labels Oct 16, 2022
@mfsiega-airbyte mfsiega-airbyte temporarily deployed to more-secrets October 16, 2022 01:34 Inactive
@mfsiega-airbyte mfsiega-airbyte temporarily deployed to more-secrets October 16, 2022 03:57 Inactive
davinchia
davinchia reviewed Oct 16, 2022
View reviewed changes
airbyte-server/src/main/java/io/airbyte/server/handlers/WorkspacesHandler.java
@@ -108,7 +105,7 @@ public WorkspaceRead createWorkspace(final WorkspaceCreate workspaceCreate)
.withTombstone(false)
.withNotifications(NotificationConverter.toConfigList(workspaceCreate.getNotifications()))
.withDefaultGeography(defaultGeography)
.withWebhookOperationConfigs(WorkspaceWebhookConfigsConverter.toPersistenceWrite(workspaceCreate.getWebhookConfigs()));
.withWebhookOperationConfigs(WorkspaceWebhookConfigsConverter.toPersistenceWrite(workspaceCreate.getWebhookConfigs(), uuidSupplier));
Copy link
Contributor

@davinchia davinchia Oct 16, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

one thought: can we rename the uuidSupplier variable to reflect this is specifically to provide a uuid for the webhook config objects? Not necessarily related to this change, however will help readability.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is not related to your change, can you add the @VisibleForTesting annotation on the construction at Line 66 so it's obvious this is for testing? Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

one thought: can we rename the uuidSupplier variable to reflect this is specifically to provide a uuid for the webhook config objects?

We do actually use it for other things -- e.g. generating new workspace ids -- so I think leaving it makes sense? (Though I'm still confused as to why we're doing that.)

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh interesting. Leaving it is good then!

davinchia
davinchia reviewed Oct 16, 2022
View reviewed changes
Copy link
Contributor

@davinchia davinchia left a comment

Choose a reason for hiding this comment

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

Nicely done.

I have some suggestions for readability & making it easier to understand how we are returning the WebhookOperations object in the API.

Although the logic looks sound to me, I want to be careful on the testing side. My understanding of the tests we have on this today:

  • test workspace writes go through the secret writer to ensure secrets are converted into coordinates.
  • test the secret writer correctly converts auth tokens
  • test the update route returns correct webhooks for patch and full updates.
  • do we test create/read routes?

Want to be clear on ^ before we merge.

@mfsiega-airbyte mfsiega-airbyte temporarily deployed to more-secrets October 16, 2022 16:53 Inactive
@mfsiega-airbyte
Copy link
Contributor Author

@davinchia thanks for the review! That's a good summary of the testing situation - I added coverage on the create and read routes in the latest revision as well. Let me know if there are any other questions/comments!

davinchia
davinchia approved these changes Oct 16, 2022
View reviewed changes
Copy link
Contributor

@davinchia davinchia left a comment

Choose a reason for hiding this comment

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

Looks good!

@mfsiega-airbyte mfsiega-airbyte merged commit e079761 into master Oct 16, 2022
@mfsiega-airbyte mfsiega-airbyte deleted the msiega/dbt-cloud-webhook-config-persistence branch October 16, 2022 23:23
@octavia-squidington-iii octavia-squidington-iii mentioned this pull request Oct 20, 2022
Merged
jhammarstedt pushed a commit to jhammarstedt/airbyte that referenced this pull request Oct 31, 2022
@mfsiega-airbyte @jhammarstedt
ensure workspace webhook configs can be persisted correctly (airbyteh…
9041591 
…q#18034)

* ensure workspace webhook configs can be correctly passed between API and persistence layers

* remove unnecessary logging

* add unit tests to workspace webhook config handling

* additional testing and style cleanup around workspace webhook config handling
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@davinchia davinchia davinchia approved these changes

Assignees
No one assigned
Labels
area/api Related to the api area/documentation Improvements or additions to documentation 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.
2 participants
@mfsiega-airbyte @davinchia