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

scope operations by workspace #4845

Merged
merged 10 commits into from
Jul 23, 2021
Merged

scope operations by workspace #4845

merged 10 commits into from
Jul 23, 2021

Conversation

cgardens
Copy link
Contributor

@cgardens cgardens commented Jul 20, 2021 •
edited
Loading

What

  • We want to scope operations by workspace (so a user only sees their own operations) and so that we can validate that a connection uses operations from the same workspace as its source and destination.

How

  • add workspace id to operations data model
  • add validations in the connection/create and connection/update

Recommended reading order

  1. config.yaml
  2. the rest

to do

  • - update migration number (will do after code review but before merge)

@github-actions github-actions bot added area/api Related to the api area/platform issues related to the platform area/documentation Improvements or additions to documentation labels Jul 20, 2021
cgardens
cgardens commented Jul 20, 2021
View reviewed changes
airbyte-api/src/main/openapi/config.yaml
@@ -952,7 +952,7 @@ paths:
content:
application/json:
schema:
$ref: "#/components/schemas/OperationCreateOrUpdate"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

clarifying naming here. This struct was used for 2 endpoints: operations/check and webackend/connections/update (which is really an update or create. the check endpoint does not need all the fields the update endpoint needs. So i split them into 2 different structs with only the fields they need and more granular names.

cgardens
cgardens commented Jul 20, 2021
View reviewed changes
airbyte-config/persistence/src/main/java/io/airbyte/config/persistence/WorkspaceFinder.java
import io.airbyte.config.SourceConnection;
import java.util.UUID;

// todo (cgardens) - this is just a utility for this PR. will need to figure out the "right" way to
Copy link
Contributor Author

Choose a reason for hiding this comment

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

as the comment says, i expect this to be thrown out once @jrhizor merges his stuff. if that's not the case i can make this better.

cgardens
cgardens commented Jul 20, 2021
View reviewed changes
airbyte-migration/src/main/java/io/airbyte/migrate/Migrations.java Outdated
@@ -55,6 +56,7 @@
private static final Migration MIGRATION_V_0_25_0 = new MigrationV0_25_0(MIGRATION_V_0_24_0);
private static final Migration MIGRATION_V_0_26_0 = new MigrationV0_26_0(MIGRATION_V_0_25_0);
private static final Migration MIGRATION_V_0_27_0 = new MigrationV0_27_0(MIGRATION_V_0_26_0);
private static final Migration MIGRATION_V_0_9000_0 = new MigrationV0_9000_0(MIGRATION_V_0_27_0);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

placeholder migration number. will update before merge.

ChristopheDuong
ChristopheDuong reviewed Jul 20, 2021
View reviewed changes
airbyte-server/src/main/java/io/airbyte/server/handlers/ConnectionsHandler.java
final StandardSync persistedSync = configRepository.getStandardSync(connectionUpdate.getConnectionId())
final StandardSync persistedSync = configRepository.getStandardSync(connectionUpdate.getConnectionId());

validateWorkspace(persistedSync.getSourceId(), persistedSync.getDestinationId(), new HashSet<>(connectionUpdate.getOperationIds()));
Copy link
Contributor

Choose a reason for hiding this comment

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

Just a comment about the order of operationIds which matters.

But I guess, here, for validation it's fine being reshuffled by the hashset...

jrhizor
jrhizor approved these changes Jul 20, 2021
View reviewed changes
Copy link
Contributor

@jrhizor jrhizor 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 % migration comment

airbyte-migration/src/main/java/io/airbyte/migrate/migrations/MigrationV0_9000_0.java Outdated
final Consumer<JsonNode> recordConsumer = outputData.get(entry.getKey());

entry.getValue().forEach(r -> {
// as of this moment it is safe to assume that every instance of airbyte has a workspace with the
Copy link
Contributor

Choose a reason for hiding this comment

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

While it is fair to assume that every instance has a workspace with the default UUID, I don't think it's fair to make the assignments that way.

Since we have users that use workspaces from the API, we should look up where operations are used (which connections) and then assign them to workspaces using that (in almost all cases this will be the default). And then if the operation isn't assigned to any connection we can assign it to the default worksapce.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good point. i'll do that.

@cgardens cgardens force-pushed the cgardens/scope_operations_by_workspace branch 2 times, most recently from 4dad526 to 5ec7d28 Compare July 23, 2021 04:55
@cgardens cgardens force-pushed the cgardens/scope_operations_by_workspace branch from 5ec7d28 to 4558891 Compare July 23, 2021 17:42
@cgardens cgardens merged commit 066fb54 into master Jul 23, 2021
@cgardens cgardens deleted the cgardens/scope_operations_by_workspace branch July 23, 2021 18:46
@inesmarcal inesmarcal mentioned this pull request Mar 30, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ChristopheDuong ChristopheDuong ChristopheDuong approved these changes

@jrhizor jrhizor jrhizor 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
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@cgardens @jrhizor @ChristopheDuong