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

datasize, blueprint: add new datasizes.Size type and use in blueprints #1057

Merged
merged 4 commits into from
Nov 22, 2024

Conversation

mvo5
Copy link
Contributor

@mvo5 mvo5 commented Nov 22, 2024

This is an alternative implementation of the idea in #1049 based on feedback from @thozza (thanks!).


blueprint: decouple FilesystemCustomization from marshaling

This commit decouples the marshaling representation of the
FilesystemCustomization from the actual struct. This means that
we can reuse custom unmarshaling via datasizes.Size and also
have our external represenation "pure".

If we continue using this pattern is also means that we can rename
fields in the marshaling and provide compatbility easily.

Thanks to Thozza, c.f.
#1049 (comment)
and
#983


datasizes: add new Size type with json/toml decoding
support

This commit adds a new datasizes.Size type that is an alias
for uint64 but has supports direct json/toml decoding. So sizes
can be specified as 123, "123" or "123 GiB" and this is transparently
handled.

@achilleas-k achilleas-k changed the title atasize, blueprint: add new datasizes.Size type and use in blueprints datasize, blueprint: add new datasizes.Size type and use in blueprints Nov 22, 2024
This commit adds a new `datasizes.Size` type that is an alias
for uint64 but has supports direct json/toml decoding. So sizes
can be specified as 123, "123" or "123 GiB" and this is transparently
handled.

datasizes: tweak error return to avoid "stuttering"

The `datasizes.Size.UnmarshalJSON()` returns a `JSON unmarshal`
prefix. However this is a bit too generic as other (nested)
unmarshalers may also use the prefix. So instead just indicate
the type in the error string itself:
```
error decoding {TOML,JSON} size: ...
```
achilleas-k
achilleas-k previously approved these changes Nov 22, 2024
mvo5 added 3 commits November 22, 2024 12:55
This commit decouples the marshaling representation of the
`FilesystemCustomization` from the actual struct. This means that
we can reuse custom unmarshaling via datasizes.Size and also
have our external represenation "pure".

If we continue using this pattern is also means that we can rename
fields in the marshaling and provide compatbility easily.

Thanks to Thozza, c.f.
osbuild#1049 (comment)
and
osbuild#983
This commit fixes the issue that minsize cannot be a string
by using the existing pattern.

See osbuild#1055 for the alternative
approach.
)

// XXX: move to interal/common ?
func unmarshalTOMLviaJSON(u json.Unmarshaler, data any) error {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This should probably grow it's own test.

Copy link
Member

@thozza thozza left a comment

Choose a reason for hiding this comment

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

Amazing! 😍

@achilleas-k achilleas-k added this pull request to the merge queue Nov 22, 2024
Merged via the queue into osbuild:main with commit ec64965 Nov 22, 2024
19 checks passed
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.

3 participants