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.

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

Fix DestinationConfigResponse struct format. #105

Closed
wants to merge 1 commit into from

Conversation

georgi0u
Copy link

@georgi0u georgi0u commented May 21, 2024

w/r/t DestinationConfigResponse:

IsPrivateKeyEncrypted comes back as a bool, not a string, and thus fails unmarshalling, as written.

See:

json: cannot unmarshal bool into Go struct field DestinationConfigResponse.data.config.is_private_key_encrypted of type string;

We — Runway.com — are depending on the fork this PR is derived from, until this is fixed.


I've read your CONTRIBUTING.md and assume one-liners are probably still welcome.

…ils unmarshalling, as written. This commit fixes that.
@georgi0u georgi0u marked this pull request as ready for review May 21, 2024 17:30
@beevital beevital self-requested a review June 4, 2024 12:59
@beevital beevital self-assigned this Jun 4, 2024
@beevital
Copy link
Collaborator

beevital commented Jun 4, 2024

@georgi0u it's better to use DoCustom and ConfigCustom (it's a map) - to avoid any issues with config types.

@georgi0u
Copy link
Author

georgi0u commented Jun 4, 2024

@georgi0u it's better to use DoCustom and ConfigCustom (it's a map) - to avoid any issues with config types.

@beevital

Can that be true? Are there variable situations in which the is_private_key_encrypted is sometimes returned as a string; and other times a bool?

My experience is that it's always returned as a bool, as this field is general to all destinations; and thus the existing/published version of that struct is broken.

To recommend using ConfigCustom here seems more of a workaround than a long term fix.

@beevital
Copy link
Collaborator

@georgi0u

To recommend using ConfigCustom here seems more of a workaround than a long term fix.

It's not a WA, it's just a common approach that we use.

I agree that is_private_key_encrypted should be marked as bool.
But the problem is that we just don't have enough resources to support this hardcoded mapping - possible config fields set changing constantly, it's just easier to deprecate and remove legacy config from request at all.

@beevital
Copy link
Collaborator

beevital commented Jun 10, 2024

And I've just checked and Snowflake returns this field in response as a string, so everything is correct here.

It's not correct on API side for sure, but we can't introduce a type change - it will brake existing contract for existing clients.
This may be fixed in next api contract version only.

@beevital beevital closed this Jun 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants