Add skips & relatedImages to CSV Spec #101
Conversation
| type: string | ||
| url: | ||
| type: string | ||
| relatedImages: |
There was a problem hiding this comment.
my only hesitation to adding these fields is that they are unused on cluster at runtime.
on the other hand, the impact is low -- the change is backwards compatible -- and there are already a few other fields of the spec that can be considered non-runtime metadata (e.g. provider, maturity, etc).
I'd like to hear what some other folks think since this is still a client facing change.
There was a problem hiding this comment.
I think it is fine to add those fields. We are using those fields implicitly on the registry side. Perhaps, it helps to make this part more transparent on the API standpoint and easier for folks to use them directly. Plus, as you said it, the change is backwards compatible so there is no risk.
There was a problem hiding this comment.
From a transparency point of view, would it make sense to add something like the following to the documentation?
This field is only used during catalog creation and plays no part in cluster runtime.
Also, do we want to add other metadata fields like provider, maturity? If so, I'd need a list of what those are. I've pieced together documentation from various sources for skips & relatedImages, so I can probably do the same for other metadata fields, assuming I know what ones to add. Also the statement I suggested above would apply to these as well.
There was a problem hiding this comment.
I don't think we want to keep adding more fields to the CSV given that they should really probably end up as properties on bundles and the CSV will go away in the future. If these are necessary for some process temporarily, I think that's fine, but we have been intentionally avoiding adding any new fields to this type for that reason.
There was a problem hiding this comment.
I wouldn't say these are new fields. These are existing fields that authors include the CSV but we simply never capture them in CSV API because they don't serve other purposes in runtime. I agree that we would like to avoid adding anything new but for this specific case, I think it is acceptable to do given the "existing" nature of the fields.
There was a problem hiding this comment.
OK. So I am hearing that this PR should only focus on skips & relatedImages since we need them for the e2e tests we are writing, and I should add This field is only used during catalog creation and plays no part in cluster runtime. to the documentation for skips & relatedImages.
There was a problem hiding this comment.
Comment updated and force pushed changes
There was a problem hiding this comment.
Is there anything else that needs to be done for this PR?
jchunkins
force-pushed
the
add_fields_csv
branch
from
ebe6906 to
644c03e
Compare
- update ClusterServiceVersionSpec to include skips & relatedImages - Both skips & relatedImages are optional - If relatedImages is included, name & image fields are required - executed "make format tidy clean vendor generate manifests" Signed-off-by: John Hunkins <jhunkins@us.ibm.com>
jchunkins
force-pushed
the
add_fields_csv
branch
from
644c03e to
de33696
Compare
|
@ecordell Any thoughts on this? |
|
/approve |
Description of the change:
skips&relatedImagesskips&relatedImagesare optionalrelatedImagesis included,name&imagefields are requiredmake format tidy clean vendor generate manifeststo ensure that the CRD and other artifacts are generated as necessary.Motivation for the change:
System level e2e tests being written for the OLM project requires the
skips&relatedImagesfields to be available on the CSV spec object when marshaling a CSV to disk so that tools such asopmcan read the CSV and act appropriately when these fields are present. If we don't add these fields, writing tests will require work arounds to make sure those values are in place so the e2e tests can work.Reviewer Checklist
/docs