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

support defining a dimension field as multi-valued #60

Merged

Conversation

guustavov
Copy link

In order to address #56, azaurus1/go-pinot-api#126 introduced singleValueField schema's property management.

The current PR refers to bumping go-pinot-api version to 0.3.0, allowing terraform provider to manage the property in question. Since this is a configuration for dimension fields only, I'm also suggesting the splitting fieldSpec in dimensionFieldSpec and metricFieldSpec.

Other than that, I also needed to refactor notNull property usage, given json encoder limitation @gbrlcustodio mentioned in azaurus1/go-pinot-api#126 when it comes to omitted falsy values.

Please be thoughtful about this being my first go at Golang 🥁, so probably this one requires a thorough review before moving on. 😅

Verified

This commit was signed with the committer’s verified signature.
legendecas Chengzhong Wu
@guustavov guustavov marked this pull request as ready for review April 1, 2024 17:47
Copy link
Owner

@azaurus1 azaurus1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @guustavov, looks good!, I have added some in-line comments for changes, the main issues are just quirks around using the types for Terraform

Verified

This commit was signed with the committer’s verified signature.
legendecas Chengzhong Wu
@guustavov guustavov requested a review from azaurus1 April 2, 2024 14:05
@azaurus1
Copy link
Owner

azaurus1 commented Apr 2, 2024

Will merge this and make a new release, thanks @guustavov !

@azaurus1 azaurus1 merged commit 9450131 into azaurus1:main Apr 2, 2024
7 of 8 checks passed
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