🪟 🎉 Multi-cloud: edit connection & workspace data geographies in the UI #18611
Conversation
josephkmh
added
area/frontend
team/compose
area/frontend
| @@ -5,6 +5,7 @@ export enum FeatureItem { | |||
| AllowUpdateConnectors = "ALLOW_UPDATE_CONNECTORS", | |||
| AllowOAuthConnector = "ALLOW_OAUTH_CONNECTOR", | |||
| AllowSync = "ALLOW_SYNC", | |||
| AllowChangeDataGeographies = "ALLOW_CHANGE_DATA_GEOGRAPHIES", | |||
There was a problem hiding this comment.
The ability to change data geographies is behind a feature flag. It's not available in OSS, and it is currently disabled in cloud. We will do a staged rollout in production to test that it's working as expected.
| /> | ||
| </div> | ||
| <div className={styles.dropdownWrapper}> | ||
| <div className={styles.spinner}>{connectionUpdating && <Spinner small />}</div> |
There was a problem hiding this comment.
I'm waiting to hear back from Nico about how we should deal with this loading state. So this loading spinner may be replaced with something else.
| @@ -39,7 +40,7 @@ interface CreateConnectionPropsInner extends Pick<CreateConnectionProps, "afterS | |||
|
|
|||
| const CreateConnectionFormInner: React.FC<CreateConnectionPropsInner> = ({ schemaError, afterSubmitConnection }) => { | |||
| const navigate = useNavigate(); | |||
|
|
|||
| const canEditDataGeographies = useFeature(FeatureItem.AllowChangeDataGeographies); | |||
There was a problem hiding this comment.
This is false for now, but can be overridden in LaunchDarkly for testing
| @@ -27,7 +27,7 @@ interface SideMenuProps { | |||
| } | |||
|
|
|||
| const Content = styled.nav` | |||
| min-width: 147px; | |||
| min-width: 155px; | |||
There was a problem hiding this comment.
The sidebar is ever-so-slightly too small for Default Data Residency. This CSS should really be more flexible, but just making it work for now!
| @@ -23,6 +24,7 @@ export const CloudSettingsPage: React.FC = () => { | |||
| // TODO: uncomment when supported in cloud | |||
| // const { countNewSourceVersion, countNewDestinationVersion } = useConnector(); | |||
| const supportsCloudDbtIntegration = useFeature(FeatureItem.AllowDBTCloudIntegration); | |||
| const supportsDefaultDataResidency = useFeature(FeatureItem.AllowChangeDataGeographies); | |||
There was a problem hiding this comment.
This is false for now, but can be overridden in LaunchDarkly for testing
josephkmh
changed the title
josephkmh
changed the title
airbyte-webapp/src/packages/cloud/views/settings/CloudSettingsPage.tsx
Outdated
Show resolved
Hide resolved
...pp/src/packages/cloud/views/workspaces/DefaultDataResidencyView/DefaultDataResidencyView.tsx
Outdated
Show resolved
Hide resolved
...pp/src/components/connection/UpdateConnectionDataResidency/UpdateConnectionDataResidency.tsx
Outdated
Show resolved
Hide resolved
...pp/src/components/connection/UpdateConnectionDataResidency/UpdateConnectionDataResidency.tsx
Outdated
Show resolved
Hide resolved
airbyte-webapp/src/locales/en.json
Outdated
| "settings.dataResidency": "Data Residency", | ||
| "settings.defaultDataResidency": "Default Data Residency", | ||
| "settings.defaultGeography": "Geography", | ||
| "settings.defaultDataResidencyDescription": "Choose the default preferred data processing location for all of your connections. Setting this choice means your data never leaves this region. <lnk>Learn more</lnk>.", |
There was a problem hiding this comment.
This is strictly spoken not completely true, and I feel this might be the place we need to clarify about that. Actually some data leaves the region. If you have incremental syncs the cursor field values, which we'll store as a "state" for the next run are always stored in our control plane (such US), no matter where the connection ran. Meaning if you chose a field as a cursor field that would anyhow have sensitive data in it (not entirely sure of a good example for that), this data will be send and stored in the US.
cc @andyjih we talked about that a while back, and I think it would be good if you could give your input on wording and where we might want to document that?
Just feels for me saying "your data will NEVER leave this region" is a bit harsh, given we know it might. Also it still depends on the connection's actually region setting (since this is only the default region screen), so we might also want to add that to the description here.
There was a problem hiding this comment.
Agreed, it's a little misleading. @andyjih also wanted to add a short clarification that the workspace default only affects new connections. How about the following?
Choose the default preferred data processing location for your connections. The default Data Residency setting only affects new Connections. Existing Connections will retain their Data Residency setting. <lnk>Learn more</lnk>.
Edit: looks like the docs PR also has some discussion on this: #18872 (comment)
| nextLine | ||
| label={<FormattedMessage id="connection.geographyTitle" />} | ||
| message={ | ||
| <FormattedMessage |
There was a problem hiding this comment.
Would it be worth to point here out, when this will take effect? I assume it's not cancelling the current running sync and I also assume it doesn't require a reset, so simply the next sync/reset jobs will be done in that region? Maybe worth putting something like: "This will take effect for all syncs and resets starting after the change." (or something better worded).
There was a problem hiding this comment.
I think this is almost self-explanatory and not necessary to explain, but maybe @andyjih can weigh in. It's also something we could consider adding to the docs instead of directly in the UI.
Here's a proposal for the wording if we do want to mention it:
Depending on your network configuration, you may need to <lnk>add IP addresses</lnk> to your allowlist. Changes to data residency will be applied to new sync and reset jobs. Currently running jobs will not be affected.
...pp/src/components/connection/UpdateConnectionDataResidency/UpdateConnectionDataResidency.tsx
Show resolved
Hide resolved
...pp/src/components/connection/UpdateConnectionDataResidency/UpdateConnectionDataResidency.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Code LGTM. I tested the feature and seems changing workspace default residency is working and affecting new connections. Updating connections is working fine. Tested on Chrome Linux.
I left some thoughts around phrasing and code nitpick. Nothing of it functional for the feature, so I'm already going ahead and approve this PR.
What
Adds data geography to workspace and connection settings.
Demo:
https://www.loom.com/share/cfc469c1e9b74e63b7e344851ddcf1dd
@natalyjazzviolin is working on a custom dropdown in a separate PR.
Note: the functionality is behind a feature flag that is currently disabled - we will do a staged rollout in production to test that it's working as intended.
How
This PR allows users to set data geography in three places:
Recommended reading order
First, the available geographies need to be fetched from the backend. I followed the existing conventions and added a GeographiesService:
packages/cloud/lib/domain/geographies/GeographiesService.tspackages/cloud/services/geographies/GeographiesService.tsThe default workspace data geography is set in the workspace settings:
packages/cloud/views/workspaces/DefaultDataResidencyView/DefaultDataResidencyView.tsxWhen creating a new connection, a new
DataResidencysection has been added:components/CreateConnection/DataResidency.tsxWhen editing an existing connection, a new
UpdateConnectionGeographyhas been added:components/UpdateConnectionGeography/UpdateConnectionGeography.tsxThe rest of the changes are related to fixing tests, styling changes, and updating interfaces to include the
workspace.defaultGeographyandconnection.geographyproperties.