🎉 Add create & delete workspaces to API #2325

gordalina · 2021-03-05T09:34:39Z

What

Closes #1735
Adds /workspaces/create and /workspaces/delete.

How

This adds tombstone to the workspace.
This also adds a check to prevent returning destinations and sources that have been tombstones.

Questions

I see that you are using TrackingClientSingleton.get().identify(); when updating a workspace. I don't think we need to do that for the new workspaces - but I leave that decision to you.

Pre-merge Checklist

Run integration tests
Publish Docker images

Recommended reading order

WorkspacesHandler

michel-tricot

The change looks great! added a few comments!

michel-tricot · 2021-03-06T04:54:00Z

airbyte-server/src/main/java/io/airbyte/server/handlers/DestinationHandler.java

+    final DestinationConnection dci = configRepository.getDestinationConnection(destinationId);
+
+    if (dci.getTombstone()) {
+      throw new NotFoundException("Could not find destination");


Can you send a ConfigNotFoundException instead?

michel-tricot · 2021-03-06T04:54:16Z

airbyte-server/src/main/java/io/airbyte/server/handlers/SourceHandler.java

+    final SourceConnection sourceConnection = configRepository.getSourceConnection(sourceId);
+
+    if (sourceConnection.getTombstone()) {
+      throw new NotFoundException("Could not find source connection");


michel-tricot · 2021-03-06T05:06:25Z

airbyte-server/src/main/java/io/airbyte/server/handlers/WorkspacesHandler.java

+    final StandardWorkspace workspace = configRepository.getStandardWorkspace(workspaceId);
+
+    if (workspace.getTombstone()) {
+      throw new NotFoundException("Could not find workspace");


Same here. We can add a boolean to getStandardWorkspace to indicate if you want to include the tombstone ones or not. This way you can let the config repo throw the ConfigNotFoundException.

WDYT?

The good thing about adding that boolean is that people will have to make the conscious choice of including tombstoned or not. Less risk of forgetting to filter out the tombstoned

michel-tricot · 2021-03-06T05:09:12Z

airbyte-server/src/main/java/io/airbyte/server/handlers/WorkspacesHandler.java

+
+  private StandardWorkspace getWorkspaceBySlug(String slug)
+      throws JsonValidationException, IOException, KnownException {
+    for (StandardWorkspace workspace : configRepository.listStandardWorkspaces()) {


Same idea for the prototype.

Instead actually. What about adding a configRepository::getStandardWorkspaceBySlug (with the tombstone boolean as well)

michel-tricot · 2021-03-06T05:19:51Z

airbyte-server/src/test/java/io/airbyte/server/handlers/WorkspacesHandlerTest.java

+
+    final WorkspaceRead created = workspacesHandler.createWorkspace(workspaceCreate);
+
+    assertTrue(created.getWorkspaceId() instanceof UUID);


The test is a bit brittle here. What about: Injecting the uuid generator in a protected constructor and mock it so you can control the value of the uuid and you can just do a assertEqual(expectedWorkspaceRead, created)

Here is an example of how we do it: io/airbyte/server/handlers/DestinationDefinitionsHandler.java:53

gordalina · 2021-03-09T02:20:13Z

@michel-tricot addressed all your comments & added a 0.17.0 migration

sherifnada · 2021-03-10T00:41:17Z

@cgardens is it too late to include this in 0.17.0 release?

michel-tricot · 2021-03-10T05:10:08Z

LGTM! You will just need to rename the migrations (we released 0.17.0 in the meantime).

michel-tricot · 2021-03-10T05:10:23Z

I'll merge it after that

cgardens

Hi Sam,

First, sorry for the slow follow up on this. There was some confusion about how was going to follow up on your PR. This was my bad.

Second, thanks for contributing this! It looks great. I have have one last round of comments. I'm hoping they are all clear and straight forward. Of course, let me know if you have any questions.

After that, I will merge this PR (for real this time--I know we said this once before already 😉 ). I don't know what your time looks like right now, so if you can't get to these, it not the end of the world. If you just say, "sorry I'm busy right now", I will just merge the PR as is and tweak the things I mentioned in my review. You've done the hard part, there's just a couple tweaks that I'm seeing based on some knowledge of some weird assumptions that we've made.

Let me know! Looking forward to getting this merged.

cgardens · 2021-03-11T21:34:58Z

airbyte-api/src/main/openapi/config.yaml

+        initialSetupComplete:
+          type: boolean
+        displaySetupWizard:
+          type: boolean


i think instead of exposing these in the API, you can hide them and just set them to false in the handler. i thin it's reasonable that these 2 fields will always be false when the workspace is created.

cgardens · 2021-03-11T21:38:13Z

airbyte-analytics/src/main/java/io/airbyte/analytics/TrackingClientSingleton.java

@@ -70,7 +70,7 @@ private static void initialize() {
  @VisibleForTesting
  static TrackingIdentity getTrackingIdentity(ConfigRepository configRepository, String airbyteVersion) {
    try {
-      final StandardWorkspace workspace = configRepository.getStandardWorkspace(PersistenceConstants.DEFAULT_WORKSPACE_ID);
+      final StandardWorkspace workspace = configRepository.getStandardWorkspace(PersistenceConstants.DEFAULT_WORKSPACE_ID, false);


to give you a little context, the concept of a "workspace" and a "deployment" are conflated in our system right now. that has been okay so far because there is only one workspace. the reason i mention it, is that we still to be able to use the default workspace for deployment info (even if the default workspace has been deleted).

all that being said, i think that just means we want true here instead of false. if the default workspace is ever deleted we still want this method to be able to access it.

cgardens · 2021-03-11T21:42:05Z

airbyte-config/models/src/main/resources/types/StandardWorkspace.yaml

@@ -9,6 +9,7 @@ required:
  - name
  - slug
  - initialSetupComplete
+  - tombstone


in the code i think you already check that if tombstone is not set that we treat it as false. i think that means that this field then doesn't need to be required. making it not required should mean that we should be able to not need to do a migration.

Up to you, if you feel like it should be required, then we can keep the migration and just drop the check you're doing in the config repository. LMK if that's not clear.

cgardens · 2021-03-11T21:44:29Z

airbyte-server/src/main/java/io/airbyte/server/handlers/ArchiveHandler.java

@@ -157,7 +157,7 @@ private void checkImport(Path tempFolder) throws IOException, JsonValidationExce

  private Optional<UUID> getCurrentCustomerId() {
    try {
-      return Optional.of(configRepository.getStandardWorkspace(PersistenceConstants.DEFAULT_WORKSPACE_ID).getCustomerId());
+      return Optional.of(configRepository.getStandardWorkspace(PersistenceConstants.DEFAULT_WORKSPACE_ID, false).getCustomerId());


same here. i think we want true. we want the default workspace to be returned even if deleted here.

cgardens · 2021-03-11T21:51:49Z

airbyte-server/src/main/java/io/airbyte/server/converters/ConfigFileArchiver.java

+    final StandardWorkspace standardWorkspace = configRepository.getStandardWorkspace(PersistenceConstants.DEFAULT_WORKSPACE_ID, false);
    if (standardWorkspace != null)
      writeConfigsToArchive(storageRoot, ConfigSchema.STANDARD_WORKSPACE, List.of(standardWorkspace));
    else


Suggested change

final StandardWorkspace standardWorkspace = configRepository.getStandardWorkspace(PersistenceConstants.DEFAULT_WORKSPACE_ID, false);

if (standardWorkspace != null)

writeConfigsToArchive(storageRoot, ConfigSchema.STANDARD_WORKSPACE, List.of(standardWorkspace));

else

writeConfigsToArchive(storageRoot, ConfigSchema.STANDARD_WORKSPACE, configRepository.listStandardWorkspaces(true));

I think we can get rid of lines 75-79 and just replace it with this. we just want to do the list of all workspaces (regardless of whether they've been deleted). github didn't let me include line 79 in the suggestion comment, so unfortunately this suggestion can't be purely committed automatically.

cgardens · 2021-03-11T21:53:02Z

airbyte-server/src/main/java/io/airbyte/server/handlers/HealthCheckHandler.java

@@ -44,7 +44,7 @@ public HealthCheckHandler(ConfigRepository configRepository) {
  public HealthCheckRead health() {
    boolean databaseHealth = false;
    try {
-      databaseHealth = configRepository.getStandardWorkspace(PersistenceConstants.DEFAULT_WORKSPACE_ID) != null;
+      databaseHealth = configRepository.getStandardWorkspace(PersistenceConstants.DEFAULT_WORKSPACE_ID, false) != null;


true here too. we don't want the healthcheck to fail because the default workspace was deleted.

cgardens

amazing! thanks for the fast turn around!

michel-tricot reviewed Mar 6, 2021

View reviewed changes

gordalina requested a review from michel-tricot March 9, 2021 02:20

michel-tricot approved these changes Mar 10, 2021

View reviewed changes

gordalina added 17 commits March 9, 2021 21:38

Do not return tombstoned sources

04310f8

Do not return tombstoned sources

14f8bc2

Add tombstone to workspaces

9353c67

Add createWorkspace

14c1dc3

Add deleteWorkspace

393d81d

Update getWorksapceBySlug & add more tests

b2458ea

Add slugify dependency

31c6b75

Lint

8a3dfbc

Add includeTombstone to workspaces query functions

311af19

Use ConfigNotFound instead of NotFoundException

2739c1f

Refactor WorkspacesHandler

50b22af

Update TrackingClientSingletonTest with new workspace api

fe8f555

Add 0.18.0 migration

73d2895

Update documentation

3d5d51b

Run formatter

01b9a45

rerun gh actions

4d0d649

Rebase fixes

cc2d75a

gordalina force-pushed the sg/workspaces branch from f780bd7 to cc2d75a Compare March 10, 2021 06:26

Update path

b373301

cgardens requested changes Mar 11, 2021

View reviewed changes

gordalina added 3 commits March 11, 2021 15:44

Remove migrations

577e81d

Do not make tombstone required in StandardWorkspace

101dc1a

Remove initialSetupComplete & displaySetupWizard from WorkspaceCreate

8ac4260

gordalina added 3 commits March 11, 2021 15:45

Use all standard workspace even if deleted

87cd187

Import & export all workspaces

b4b0d9d

Update documentation

6a2ce04

gordalina requested a review from cgardens March 11, 2021 23:46

cgardens approved these changes Mar 11, 2021

View reviewed changes

cgardens merged commit 73044ac into airbytehq:master Mar 11, 2021

cgardens changed the title ~~Add create & delete workspaces to API~~ 🎉 Add create & delete workspaces to API Mar 12, 2021

cgardens changed the title ~~🎉 Add create & delete workspaces to API~~ 🎉 Add create & delete workspaces to API Mar 12, 2021

ChristopheDuong mentioned this pull request Mar 30, 2021

Migration from 0.17.1-alpha to 0.18.0-alpha fails #2665

Closed


		final WorkspaceRead created = workspacesHandler.createWorkspace(workspaceCreate);

		assertTrue(created.getWorkspaceId() instanceof UUID);

🎉 Add create & delete workspaces to API #2325

🎉 Add create & delete workspaces to API #2325

Uh oh!

Conversation

gordalina commented Mar 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What

How

Questions

Pre-merge Checklist

Recommended reading order

Uh oh!

michel-tricot left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gordalina commented Mar 9, 2021

Uh oh!

sherifnada commented Mar 10, 2021

Uh oh!

michel-tricot commented Mar 10, 2021

Uh oh!

michel-tricot commented Mar 10, 2021

Uh oh!

cgardens left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cgardens Mar 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cgardens left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

gordalina commented Mar 5, 2021 •

edited

Loading

cgardens Mar 11, 2021 •

edited

Loading