-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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.
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
introduce persistence code for service account table #11944
introduce persistence code for service account table #11944
Conversation
type: string | ||
serviceAccountEmail: | ||
type: string | ||
jsonCredential: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick. It would be helpful to add a sample json credential here, or just add a reference to the mock_serivce_key.json
file. Sometimes it is hard to find how a json column should look like.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could this just be a string type? I.e. do we ever actually want to deserialize the cred; my understanding is that the fact that it's a JSON blob is just an implementation detail of GCP and we shouldn't need to care about it
regardless, I do agree that an example would be useful for future reference
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
^ agree with this. The pro for having a json column is that it is easier to search. However, I imagine that in reality we seldom have to search anything from the credentials. So there is not much benefit from storing the json type here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have added a comment for reference to the example, I think there is more value in keeping this as JsonNode data type cause it makes sure that the data type is in sync with the database + the key of service account is in JSON format
description: Integration specific blob. Must be a valid JSON string. | ||
type: object | ||
existingJavaType: com.fasterxml.jackson.databind.JsonNode | ||
hmacKey: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick. Same for the hmac key. Would be nice to have a sample json here.
…-persistence-code-for-service-account-table # Conflicts: # airbyte-config/persistence/src/main/java/io/airbyte/config/persistence/DbConverter.java
* implement secret handling for workspace_service_account table * add new line to the mock json * get rid of file * address review comments * update method name and add comment
* implement migration to create workspace_service_account table * make all columns non nullable * introduce persistence code for service account table (#11944) * implement persistence code for workspace_service_account table * update yaml * implement secret handling for workspace_service_account table (#11946) * implement secret handling for workspace_service_account table * add new line to the mock json * get rid of file * address review comments * update method name and add comment
* implement migration to create workspace_service_account table * make all columns non nullable * introduce persistence code for service account table (#11944) * implement persistence code for workspace_service_account table * update yaml * implement secret handling for workspace_service_account table (#11946) * implement secret handling for workspace_service_account table * add new line to the mock json * get rid of file * address review comments * update method name and add comment
Issue : #11968