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

try to render some changes between "" and null #34740

Merged
merged 1 commit into from
Feb 28, 2024

Conversation

jbardin
Copy link
Member

@jbardin jbardin commented Feb 27, 2024

Changes between empty strings and null were hidden in the CLI output, because the SDK could not reliably detect the difference and may return either value depending on the situation.

This legacy behavior can be confusing for authors of new providers which can correctly handle null, and it would be preferable to be able to render those changes in the CLI.

While we don't have enough information to detect when the legacy behavior is required, we can detect a number of cases where it's certain that we are not dealing with a legacy schema and should output the full diff:

  • Anything with nested structural attributes
  • Blocks of type NestingMap and NestingGroup
  • Dynamic types

While this cannot solve all issues when the resource only uses primitive types, like with random_password in #31887, it would for example solve the terraform_data example because of it's use of DynamicPseudoType.

Changes between empty strings and `null` were hidden in the CLI output,
because the SDK could not reliably detect the difference and may return
either value depending on the situation.

This legacy behavior can be confusing for authors of new provider which
can correctly handle `null`, and it would be preferable to be able to
render those changes in the CLI.

While we don't have enough information to detect when the legacy
behavior is required, we can detect a number of cases where it's
certain that we are not dealing with a legacy schema and should output
the full diff.
@jbardin jbardin force-pushed the jbardin/render-string-null-changes branch from 5afd681 to bdd5a2c Compare February 27, 2024 21:59
@jbardin jbardin requested a review from a team February 27, 2024 22:09
@jbardin jbardin marked this pull request as ready for review February 27, 2024 22:15
Copy link
Member

@bflad bflad left a comment

Choose a reason for hiding this comment

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

I don't have full context on the implications of this sort of change, but from the proposed changes here, it feels like a relatively lightweight enough and pragmatic way to help the renderer in this situation without going the full route of encoding the "legacy type system" flag into the plan. We can always back this change out if there are concerns or issues. All the code is internal and it only affects the human-readable plan output, not the machine-readable plan output that has compatibility concerns.

@jbardin jbardin merged commit dada9d6 into main Feb 28, 2024
6 checks passed
@jbardin jbardin deleted the jbardin/render-string-null-changes branch February 28, 2024 18:31
Copy link

Reminder for the merging maintainer: if this is a user-visible change, please update the changelog on the appropriate release branch.

Copy link

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 30, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants