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

pkg: make seed in ImageType.Manifest() random by default #1107

Merged
merged 1 commit into from
Dec 20, 2024

Conversation

mvo5
Copy link
Contributor

@mvo5 mvo5 commented Dec 13, 2024

This commit changes the API of ImageType.Manifest() to take a pointer to the seed value. If no seed value is given it will automatically generate a random one.

This avoid the pitfall that the consumer needs to know about properly seeding the manifests. The default use-case is to generate non-fixed seed manifests so this commit makes it easier.

[edit: this is one possible way to avoid https://github.com/osbuild/image-builder-cli/pull/6#discussion_r1882807289]

This commit changes the API of `ImageType.Manifest()` to take
a pointer to the seed value. If no seed value is given it will
automatically generate a random one.

This avoid the pitfall that the consumer needs to know about
properly seeding the manifests. The default use-case is to
generate non-fixed seed manifests so this commit it easier.
@schutzbot
Copy link
Contributor

This PR changes the images API or behaviour causing integration failures with osbuild-composer. The next update of the images dependency in osbuild-composer will need work to adapt to these changes.

This is simply a notice. It will not block this PR from being merged.

Copy link
Member

@ondrejbudai ondrejbudai left a comment

Choose a reason for hiding this comment

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

I like that it's safe by default.

Comment on lines +125 to +126
// A custom seed for the rng can be specified, if nil the seed will
// be random.
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we can be more accurate and informative here and say that it will be derived from the current time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! I'm happy to do that, I was thinking of using crypto.Rand to seed the generator but maybe I'm overthinking it. Happy to either document or change.

Copy link
Member

Choose a reason for hiding this comment

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

Not important. Can do later or not at all.

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.

Thanks, this looks OK to me.

@mvo5 mvo5 changed the title [RFC] pkg: make seed in ImageType.Manifest() random by default pkg: make seed in ImageType.Manifest() random by default Dec 19, 2024
@mvo5
Copy link
Contributor Author

mvo5 commented Dec 20, 2024

I prepared the followups to fix the consumers

Merged via the queue into osbuild:main with commit cabe041 Dec 20, 2024
22 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.

5 participants