Skip to content

Conversation

@666Goofy666
Copy link
Contributor

Contributor's Note

  • I have added / updated documentation in /docs for any user-facing features or additions.
  • I have added / updated acceptance tests in /fwprovider/tests for any new or updated resources / data sources.
  • I have ran make example to verify that the change works as expected.

Note

Feel free to change the code, especially the tests. QA and tests are always stronger when written by someone other than the original author. I just needed something to validate my functions, so improvements are welcome!

The Proxmox API returns very large datastore sizes (petabytes) in scientific notation, which was breaking unmarshaling into CustomInt64. This PR adds support for those scientific notation values.

Proof of Work

Make a fake big datastore :

truncate -s 1P /root/fakefs.img mkdir -p /mnt/fake_big_xfs mount -o loop /root/fakefs.img /mnt/fake_big_xfs

In GUI : Datacenter → Storage → Add → Directory / Directory : /mnt/fake_big_xfs

Before patch I reproduce issue : #2298

After the patch, parsing large storage values looks correct: (make example)

 "enabled" = true "id" = "fake_big_xfs" "node_name" = "pve" "shared" = false "space_available" = 1104339582869504 "space_total" = 1125897765650430 "space_used" = 21558182780928 "space_used_fraction" = 0.0191475491280275 

For comparison :

df -B1 /mnt/fake_big_xfs Filesystem 1B-blocks Used Available Use% Mounted on /dev/loop0 1125897765650432 21558182780928 1104339582869504 2% /mnt/fake_big_xfs 

Community Note

  • Please vote on this pull request by adding a 👍 reaction to the original pull request comment to help the community and maintainers prioritize this request
  • Please do not leave "+1" or other comments that do not add relevant new information or questions, they generate extra noise for pull request followers and do not help prioritize the request

Closes #0000 | Relates #0000

Signed-off-by: 666Goofy666 <153852747+666Goofy666@users.noreply.github.com>
@gemini-code-assist
Copy link
Contributor

Summary of Changes

Hello @666Goofy666, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request enhances the resilience of the CustomInt and CustomInt64 types by improving their JSON unmarshaling capabilities. The primary goal is to correctly interpret large integer values, such as petabyte-scale storage sizes, that the Proxmox API might return in scientific notation. This change prevents data parsing errors and ensures accurate representation of these values within the provider, supported by new unit and acceptance tests.

Highlights

  • Improved JSON Unmarshaling: The CustomInt and CustomInt64 types now robustly handle numeric values, including those expressed in scientific notation, when unmarshaling JSON. This specifically addresses an issue where large datastore sizes from the Proxmox API, often returned in scientific notation, would cause unmarshaling failures.
  • Enhanced Error Handling: The UnmarshalJSON methods for CustomInt and CustomInt64 have been updated to provide more specific error messages when encountering unsupported numeric formats.
  • Comprehensive Testing: New unit tests have been added for both CustomInt and CustomInt64 to cover a wide range of scenarios, including plain integers, quoted integers, scientific notation, negative values, float truncation, and null pointers. An acceptance test for datastore attributes has also been introduced.

🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console.

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature Command Description
Code Review /gemini review Performs a code review for the current pull request in its current state.
Pull Request Summary /gemini summary Provides a summary of the current pull request in its current state.
Comment @gemini-code-assist Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help /gemini help Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Footnotes

  1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution.

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request improves the robustness of UnmarshalJSON for CustomInt and CustomInt64 by adding a fallback to parse scientific notation and floating-point numbers. This is a good improvement to handle large values from the Proxmox API.

I've added a couple of suggestions:

  • For CustomInt64, I've recommended using math/big.Float to avoid potential precision loss when parsing very large numbers that are represented in scientific notation. This is important for maintaining data integrity for values like datastore sizes.
  • For CustomInt, I've suggested a small change to make the integer parsing more portable across different platform architectures (32-bit vs 64-bit).

The added unit tests are comprehensive and cover many edge cases, which is great. The acceptance test for the datastores data source is also a good addition. Overall, this is a solid contribution.

Using math/big.Float for parsing prevent precision loss, and use a bit size of 0, which will make ParseInt use the platform's native integer size. Signed-off-by: 666Goofy666 <153852747+666Goofy666@users.noreply.github.com>
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request improves the robustness of CustomInt and CustomInt64 UnmarshalJSON methods by adding support for scientific notation, which is a great improvement. The implementation using math/big is solid, and the new unit tests provide good coverage. I've added one suggestion to make the tests for CustomInt architecture-independent, ensuring they run correctly on both 32-bit and 64-bit systems.

@gemini-code-assist
Copy link
Contributor

Thank you for the summary! I'm now proceeding with a detailed review of the changes and will provide my feedback shortly.

Signed-off-by: 666Goofy666 <153852747+666Goofy666@users.noreply.github.com>
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request addresses an issue with unmarshaling large integer values from the Proxmox API that are represented in scientific notation. The changes in CustomInt and CustomInt64 UnmarshalJSON methods are robust, correctly handling this by falling back to big.Float for parsing. The addition of comprehensive unit tests is excellent and ensures the correctness of the fix across various numeric formats and edge cases. I have one minor suggestion to improve a comment for better clarity.


*r = CustomInt64(i)
// If that fails, numbers may be provided in scientific notation or as floats.
// Parse as float64 and convert to int64 (truncate towards zero).
Copy link
Contributor

Choose a reason for hiding this comment

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

medium

The comment mentions parsing as float64, but the implementation correctly uses big.Float for arbitrary-precision parsing. Using float64 would lead to precision loss for large integers, which this change is meant to handle. To avoid confusion for future maintainers, it would be clearer to update the comment to reflect that a high-precision float is used.

Suggested change
// Parse as float64 and convert to int64 (truncate towards zero).
// Parse as a high-precision float and convert to int64 (truncating towards zero).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

1 participant