-
Notifications
You must be signed in to change notification settings - Fork 584
clusterversion: Rename 'Payload' to 'Image' #175
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
clusterversion: Rename 'Payload' to 'Image' #175
Conversation
|
I think we should also change the Cincinnati API since the only thing we will return will be an image. @abhinavdahiya @crawford @steveej I have corresponding PRs primed for CVO and origin to make the corresponding changes - if we agree on this I'll make sure they're ready and then we'll merge all of these in sequence. |
The contents of an update must be an image ref. We should name the field what it is. Depends on openshift/api#175 merging first, then we can bump and go.
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: abhinavdahiya, smarterclayton The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
steveej
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm fine with replacing Payload by Image, though it doesn't help much when talking about these things out of context. I think at least in the docs and comments we should prefix that with (r|R)elease, maybe even in the variables.
More importantly I found a language nitpick.
config/v1/types_cluster_version.go
Outdated
| // desired is the version that the cluster is reconciling towards. | ||
| // If the cluster is not yet fully initialized desired will be set | ||
| // with the information available, which may be a payload or a tag. | ||
| // with the information available, which may be a image or a tag. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/a image/an image
|
Agree on using |
We consistently use the "release image" terminology in our commands and design docs when we describe the artifact that carries our manifests in an update. Payload predates that term (from Omaha) and general consensus is that we have no plans to deliver our updates in any form other than as an OCI compatible image. Update the cluster version object to make it clear that an API consumer must interpret the payload as a standard Docker image, as they already do.
8da1ce1 to
cefbd60
Compare
|
Any other comments? If none by tomorrow I'm going to merge and manage propagation. |
|
LGTM |
|
Tagging, other PRs coming up shortly which I will ask for review directly on each. |
|
/refresh |
The contents of an update must be an image ref. We should name the field what it is. Depends on openshift/api#175 merging first, then we can bump and go.
The contents of an update must be an image ref. We should name the field what it is. Depends on openshift/api#175 merging first, then we can bump and go.
The contents of an update must be an image ref. We should name the field what it is. Depends on openshift/api#175 merging first, then we can bump and go.
The contents of an update must be an image ref. We should name the field what it is. Depends on openshift/api#175 merging first, then we can bump and go.
We consistently use the "release image" terminology in our commands
and design docs when we describe the artifact that carries our manifests
in an update. Payload predates that term (from Omaha) and general
consensus is that we have no plans to deliver our updates in any form
other than as an OCI compatible image.
Update the cluster version object to make it clear that an API consumer
must interpret the payload as a standard Docker image, as they already
do.