Conversation
gszpak
left a comment
There was a problem hiding this comment.
Overall LGTM - one thing I'm not a huge fan of is the create_dataset flow. The logic to use the default signer (or be able to pass the signer) should be part of createDataset mutation and handled by the backend
| f"IAMIntegration {validation_result['validateDataset']['checks']['name']} was not successfully added added to the project." | ||
| ) | ||
| except Exception as e: | ||
| dataset.delete() |
There was a problem hiding this comment.
Just wondering: can we add optional SignerId to createDatasetInput? This way we wouldn't have to do this rollback, it would all be handled by the backend
| """mutation validateDatasetPyApi($id: ID!){validateDataset(where: {id : $id}){ | ||
| valid checks{name, success}}} | ||
| """, {'id': dataset.uid}) | ||
| if not validation_result['validateDataset']['checks'][0]['success']: |
There was a problem hiding this comment.
Are we sure the IAM validation will always be at index 0? Why aren't we checking all results?
There was a problem hiding this comment.
checks[0] is the dataset validation and it will always exist. Checks[1:] includes checks to see if each data row is valid and since there are no data rows we only need to check checks[0]. But also looking into this more, validation_result['validateDataset']['valid'] returns true only if all checks are true. So I'll use that instead.
| provider = Field.String("provider") | ||
| valid = Field.Boolean("valid") | ||
| last_valid_at = Field.DateTime("last_valid_at") | ||
| is_org_default = Field.Boolean("is_org_default") |
There was a problem hiding this comment.
seems like we're missing settings field?
There was a problem hiding this comment.
ugh - I know. The union thing is a nightmare. But users might want to know which bucket they have access to. I'll try to figure something out.
| """ | ||
|
|
||
| def __init__(self, client, data): | ||
| self.settings = data.pop('settings', {}) |
There was a problem hiding this comment.
instead of keeping a dict, could we map it to a dataclass based on __typename? (Or to Entity - not sure how we represent "helper" gql types like eg GcpIamIntegrationSettings?)
No description provided.