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

Handle empty strings in config values #700

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

tobio
Copy link
Member

@tobio tobio commented Aug 30, 2023

Description

Empty strings are treated as null in the underlying API. Mimic that behaviour on the provider schema.

Related Issues

Fixes #698

Motivation and Context

How Has This Been Tested?

Unit testing, manually.

Types of Changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Refactoring (improves code quality but has no user-facing effect)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation

Readiness Checklist

  • My code follows the code style of this project
  • My change requires a change to the documentation
  • I have updated the documentation accordingly
  • I have added tests to cover my changes
  • All new and existing tests passed

@tobio tobio requested review from wandergeek and dimuon August 30, 2023 12:53
@tobio tobio self-assigned this Aug 30, 2023
@tobio tobio requested a review from a team as a code owner August 30, 2023 12:53
Copy link
Contributor

@dimuon dimuon left a comment

Choose a reason for hiding this comment

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

LGTM 👍

Changelog is needed.

It seems we face with the similar issue for different fields when an empty value can be represented by either null or empty value. Still we came up with different solutions, e.g. as far as I can recall, for some fields we check the value specified in the config/plan and fallback to it in the reader, while here we change the next planned value to null in plan modifier.
Both approaches seem valid but maybe it makes sense to stick to the same way for different fields, also maybe we can come up with a general solution.
Anyway please feel free to merge the PR.

@syepes
Copy link

syepes commented Jan 17, 2024

+1

@Zawadidone
Copy link

+1, @tobio when will this PR be reviewed and merged? It could solve an issue that randomly occurs when the version is an empty string (#698 (comment)).

@tobio
Copy link
Member Author

tobio commented Feb 4, 2024

@Zawadidone sorry for leaving this one sitting around. I'm wary there's still issues with the overall config attribute and have wanted to do some more testing on this but it's unfortunately been dropped for a little while.

I'll try and refresh this one and assuming it's not introducing another bug get it merged in the next few weeks.

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.

Provider produced inconsistent result after apply (es and kibana config yamls)
4 participants