Remove SaaS type enum #1197
Remove SaaS type enum #1197
Conversation
Update tests to dynamically compare results rather than looking for static values.
| assert ( | ||
| len(data) | ||
| == len(ConnectionType) + len(saas_template_registry.connector_types()) - 4 | ||
| ) # there are 4 connection types that are not returned by the endpoint |
There was a problem hiding this comment.
Which connections types are not returned?
There was a problem hiding this comment.
we filter out these 5 connection types here:
ConnectionType.saas,
ConnectionType.https,
ConnectionType.manual,
ConnectionType.email,
ConnectionType.manual_webhook,
but then re-add manual_webhook here. i'm not 100% sure why it works this way.
obviously this piece of the test is a bit less dynamic and less robust to changes moving forward. but it seems reasonable to me that the test would need to be updated when that code changes - it's a substantive functionality change, of a different variety than simply adding in a new saas connector.
There was a problem hiding this comment.
I agree, this is completely reasonable, it will reduce the amount of times we have to come visit these tests and that's all we need
| @@ -152,22 +246,34 @@ def test_search_system_type(self, api_client, generate_auth_header, url): | |||
| resp = api_client.get(url + "?system_type=saas", headers=auth_header) | |||
| assert resp.status_code == 200 | |||
| data = resp.json()["items"] | |||
| assert len(data) == 14 | |||
| assert len(data) == len(saas_template_registry.connector_types()) | |||
| if saas_type != SaaSType.custom and is_match(saas_type.value) | ||
| saas_type | ||
| for saas_type in registry.connector_types() | ||
| if saas_type != "custom" and is_match(saas_type) |
There was a problem hiding this comment.
Now that we're moving away from the SaaSType enum we can stop using references to custom. It was originally meant to identify connector types that weren't in the enum list but now that any identifier is valid there is no need for this convention.
There was a problem hiding this comment.
good call! forgot about this consequence of the change.
Since we no longer have an enum for SaaSType, we don't need to have a specific 'custom' type - instead, users can simply create their own type dynamically. There is also now no more invalid 'type' value for saas configs, so we remove the test for that functionality.
| @@ -36,6 +36,7 @@ The types of changes are: | |||
| * Changed behavior of `load_default_taxonomy` to append instead of upsert [#1040](https://github.com/ethyca/fides/pull/1040) | |||
| * Changed behavior of adding privacy declarations to decouple the actions of the "add" and "next" buttons [#1086](https://github.com/ethyca/fides/pull/1086) | |||
| * Moved system related UI components from the `config-wizard` directory to the `system` directory [#1097](https://github.com/ethyca/fides/pull/1097) | |||
| * Updated "type" on SaaS config to be a simple string type, not an enum [#1197](https://github.com/ethyca/fides/pull/1197) | |||
There was a problem hiding this comment.
are we updating CHANGELOG's for now, while we are still merging over? i assume so...
|
follow up issues to make frontend fully dynamic handling of |
Remove SaaS type enum on the backend
Front end should still be working as it did before. Actually, it will now get back all connection types that are registered via the
saas_connector_registry.toml. I haven't yet confirmed that on unified fides because of general front-end issues (I think), so I want to hold on merging this PR here until we can confirm that. I had confirmed it onfidesops.There is also further UI work, beyond the scope of this PR/ticket, to refactor such that no SaaSTypes are enumerated/hardcoded, and instead just populated dynamically via API calls to the backend. Those tickets to cover that future work will be created soon.
(Note: that i'm waiting on finalizing this PR until there's some clarity on #1195, because as part of this work, I wanted to make sure our server was loading the saas registry on server initialization and did not need to reload the registry from disk when an API call is made to get connection types. see this commit from fidesops ethyca/fidesops@2ad4a2b, which is no longer applicable, for more context)I think we're OK to move forward with this even though #1195 is outstanding. it does seem like saas connector registration has been moved to a startup hook (rather than being run before app startup as it was before in
fidesops) - as long as that's the case, the saas registry should be properly loaded on server initialization, and we won't need to reload from disk upon the first API invocation.Closes #1196
Code Changes
Steps to Confirm
Pre-Merge Checklist
CHANGELOG.mdDescription Of Changes
Write some things here about the changes and any potential caveats