Skip to content

add support for binary_data field in config_map_v1_data #2616

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

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

JaylonmcShan03
Copy link
Contributor

@JaylonmcShan03 JaylonmcShan03 commented Nov 4, 2024

Description

Fixes #2467
This PR enhances the kubernetes_config_map_v1_data resource by introducing the binary_data field

Acceptance tests

  • Have you added an acceptance test for the functionality being added?
  • Have you run the acceptance tests on this branch?

Output from acceptance testing:

$ make testacc TESTARGS='-run=TestAccXXX'
test_name=TestAccKubernetesConfigMapV1Data_binaryData
--- PASS: TestAccKubernetesConfigMapV1Data_binaryData (2.89s)
PASS
ok      github.com/hashicorp/terraform-provider-kubernetes/kubernetes   3.695s
jaylon.mcshan@jaylon terraform-provider-kubernetes % 
...

Release Note

Release note for CHANGELOG:

...

References

Community Note

  • Please vote on this issue by adding a 👍 reaction to the original issue to help the community and maintainers prioritize this request
  • If you are interested in working on this issue or have submitted a pull request, please leave a comment

@JaylonmcShan03 JaylonmcShan03 requested a review from a team as a code owner November 4, 2024 18:01
@github-actions github-actions bot added the size/M label Nov 4, 2024
Copy link
Member

@alexsomesan alexsomesan left a comment

Choose a reason for hiding this comment

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

It seems that data attribute is still required regardless of binary_data being set or not. IMO, it makes more sense to have an OR type of relationship. There is a use-case where a CM would only contain binary data (like ones produced by kubectl create cm 'test' --from-file=...).

However, more importantly, destroying the resource doesn't remove the binary_data items, but does remove the ones under data. We should stay consistent across the two.

@JaylonmcShan03
Copy link
Contributor Author

@alexsomesan Regarding this portion of feedback "destroying the resource doesn't remove the binary_data items, but does remove the ones under data. We should stay consistent across the two."

Are you stating currently for the data attr we are Extracting / tracking which data entries are managed by terraform, and stripping unmanaged data entries, and for the binary_data attr that logic isn't in place. As a result, when the resource is destroyed, the binary_data items remain unchanged?

@alexsomesan alexsomesan force-pushed the add-binary-data-support-to-config-map-data branch from 1fd1570 to 78a2403 Compare May 7, 2025 14:45
@alexsomesan alexsomesan requested a review from a team as a code owner May 7, 2025 14:45
@alexsomesan
Copy link
Member

@alexsomesan Regarding this portion of feedback "destroying the resource doesn't remove the binary_data items, but does remove the ones under data. We should stay consistent across the two."

Are you stating currently for the data attr we are Extracting / tracking which data entries are managed by terraform, and stripping unmanaged data entries, and for the binary_data attr that logic isn't in place. As a result, when the resource is destroyed, the binary_data items remain unchanged?

I just din a hands-on test and it looks like the destroy functionality performs as expected. Setting the binary_data field to empty block is equivalent to removing it from the API.
Disregard my observations about the destroy behavior.

alexsomesan
alexsomesan previously approved these changes May 16, 2025
Copy link
Member

@alexsomesan alexsomesan left a comment

Choose a reason for hiding this comment

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

@JaylonmcShan03 I tested this change specifically for the observation I made about delete and it looks like it's working as expected. Setting the value of the binary_data attribute to empty or nil has an effect equivalent to delete.

In short, looks good. Let's ship it :)

@JaylonmcShan03 JaylonmcShan03 force-pushed the add-binary-data-support-to-config-map-data branch from 78a2403 to 83c1e14 Compare May 19, 2025 15:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow inputting b64 encoded data with kubernetes_config_map_v1_data via binary_data
3 participants