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

Set ImageContent.Download as required #270

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

itzikb-redhat
Copy link
Contributor

Fixes: #261

@github-actions github-actions bot added the semver:major Breaking change label Feb 24, 2025
Comment on lines -232 to -233
// +optional
Download *ImageContentSourceDownload `json:"download,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

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

There are a couple of things to think of here.

Firstly, I had it in mind that we might end up implementing the copy-image flow, which I understand is widely used in some quarters. That would require a second struct here, and we'd probably want to turn this back into a pointer at that point. However, if this were the only concern, simply making this required but still a pointer would be fine: we can 'upgrade' from that without breaking either semver or kube backwards compatibility.

The other issue is more complex, though. One of the earliest use cases we discussed for ORC, but we haven't implemented yet, is 'import and manage'. This is for when we want ORC to take ownership of resources which already exist. In that case we cannot define download, and we would definitely want it to be empty. We would also have to define the validation for that at the Spec level, so both the context of 'import and manage' and the resource spec are in scope.

I'm not sure this is straightforward.

Choose a reason for hiding this comment

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

This strikes me that perhaps you don't actually want this field to be required?

@itzikb-redhat itzikb-redhat force-pushed the image-download-required branch from f94ee17 to b48a50d Compare February 25, 2025 10:07
@github-actions github-actions bot added semver:patch No API change and removed semver:major Breaking change labels Feb 25, 2025
@itzikb-redhat itzikb-redhat force-pushed the image-download-required branch 2 times, most recently from fc29baf to 823c486 Compare February 25, 2025 12:09
@itzikb-redhat itzikb-redhat force-pushed the image-download-required branch from 823c486 to e26cd70 Compare February 25, 2025 13:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver:patch No API change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

No validation prevents creating image without source URL
3 participants