Skip to content

Conversation

analytically
Copy link
Contributor

@analytically analytically commented Mar 16, 2025

Refactor memory unit parsing to improve code clarity and maintainability.
The existing implementation was already using the correct binary multipliers
(2^10, 2^20, 2^30) but with a less clear approach of using powers.

Add support for IEC standard notation (KiB, MiB, GiB) alongside the traditional
units (KB, MB, GB) and abbreviated forms (K, M, G). Fix case sensitivity
issues with a proper case-insensitive regex pattern.

@analytically analytically requested a review from a team as a code owner March 16, 2025 16:12
@analytically analytically changed the title atc: fix incorrect memory unit multipliers in container limits atc: enhance container memory unit parsing with IEC notation support Mar 16, 2025
Add support for IEC standard notation (KiB, MiB, GiB) alongside the existing
traditional units (KB, MB, GB). Improve the regex pattern to handle abbreviated
units (K, M, G) and ensure case insensitivity across all unit formats. No
functional change to existing behavior - all units continue to use binary
multipliers (2^10, 2^20, 2^30) as before.

Signed-off-by: Mathias Bogaert <mathias.bogaert@gmail.com>
@analytically
Copy link
Contributor Author

Should we validate limits too? Like there doesn't seem a point in setting limits < 256kb?

Copy link
Member

@taylorsilva taylorsilva left a comment

Choose a reason for hiding this comment

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

Docs should be updated: https://github.com/concourse/docs/blob/c7b8d360dfe2ca7ddef04b359dcfa42a6b8d7811/lit/docs/tasks.lit#L287-L290

They don't even mention the current state correctly of being able to use KB/MB/KB

@taylorsilva taylorsilva merged commit 140347b into concourse:master May 12, 2025
12 checks passed
@github-project-automation github-project-automation bot moved this from Todo to Done in Pull Requests May 12, 2025
taylorsilva added a commit to concourse/docs that referenced this pull request May 12, 2025
Found this wasn't documented while reviewing
concourse/concourse#9130

Signed-off-by: Taylor Silva <dev@taydev.net>
@analytically analytically deleted the memlimits branch May 12, 2025 17:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants