-
Notifications
You must be signed in to change notification settings - Fork 53
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
(e2e) Use custom built test catalog for e2e testing #250
Conversation
I think this is overkill. We should be able to simply build the images, |
I think we should emulate the way rukpak set this up: Directly refer to those image in e2e's. For example: https://github.com/operator-framework/rukpak/blob/fdc3ebd85c928072fc6f6b59a2debf752159b791/test/e2e/plain_provisioner_test.go#L71 |
This feels very heavy handed - is there a reason we have to stand up an image registry on cluster instead of just In my head the most simple and straightforward approach to using a custom built test catalog would be to:
|
Looks like my use of the
TIL. Updated the PR to just use built and loaded images |
ae4519e
to
0532f43
Compare
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.
Overall looks good to me, only have a question about the related images in the bundle blob
Removed the related images, they aren't needed 👍🏽 |
/hold looks like the e2e tests are failing after rebasing on top of #216, have to investigate it |
testdata/bundles/registry-v1/prometheus-operator.v0.47.0/metadata/annotations.yaml
Outdated
Show resolved
Hide resolved
{ | ||
"schema": "olm.package", | ||
"name": "prometheus", | ||
"defaultChannel": "beta" |
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'd like to remove this line as well based on my other comment, but I think catalogd may validate the input (and removing this line would cause that validation to fail)
Maybe add a TODO comment to this line that we're only setting this to satisfy the existing OLMv0-based validation logic and that we should look for a way to be able to remove this. WDYT?
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.
Looks like adding comments to a json file isn't that easy.
One way would be to add a key/value of the form "comment": "TODO:...", but that's possibly a bit confusing?
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.
Oh yeah, forgot about that. Let's just leave it as is.
dd41d0b
to
2609831
Compare
/hold cancel |
… tests Signed-off-by: Bryce Palmer <bpalmer@redhat.com>
… tests Signed-off-by: Bryce Palmer <bpalmer@redhat.com>
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.
All this seems fine for now. There is likely going to be some modifications as part of #242
"relatedImages": [ | ||
{} | ||
] |
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.
Nit: remove "relatedImages
" entirely?
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.
It looks like spec.relatedImages field is not optional:
condition "Unpacked" is False/UnpackFailed; message: "create bundle metadata objects: applying bundle metadata \"test-catalog-prometheusoperator.0.47.0\": BundleMetadata.catalogd.operatorframework.io \"test-catalog-prometheusoperator.0.47.0\" is invalid: spec.relatedImages: Invalid value: \"null\": spec.relatedImages in body must be of type array: \"null\""
Admittedly I'd have preferred to keep it at "relatedImages": []
but even that is read as null, and had to do some json jujitsu to come up with [{}]
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.
^ Shouldn't be an issue after operator-framework/catalogd#38 is done
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.
This is a bug in catalogd. We should not require relatedImages
to be populated. In fact, I would argue that [{}]
is more invalid because it is saying "here's a related image. it's image is ""
and it's name is ""
.
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.
We'll have to create a follow up issue to fix this.
closes operator-framework#215 Signed-off-by: Anik <anikbhattacharya93@gmail.com>
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.
/approve
… tests Signed-off-by: Bryce Palmer <bpalmer@redhat.com>
… tests Signed-off-by: Bryce Palmer <bpalmer@redhat.com>
… tests Signed-off-by: Bryce Palmer <bpalmer@redhat.com>
… tests Signed-off-by: Bryce Palmer <bpalmer@redhat.com>
This PR
Stands up a local image registry in the cluster
Builds bundles and catalog images and uploads them to the image registry
Uses the custom images in the e2e test suite
Also introduces a
pullSecret
field for the Operator API's Spec struct, to allow installation of operators on cluster whose bundles require imagePullSecret to be provisioned. This is required because the bundle images built and uploaded to the local registry above requires a pull secret for the local registry.closes #215
Description
Reviewer Checklist