OPRUN-3795: Initial implementation for catalog consistency tests #320
Conversation
camilamacedo86
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@camilamacedo86: This pull request references OPRUN-3795 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.19.0" version, but no target version was set. In response to this: Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
camilamacedo86
changed the title
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: camilamacedo86 The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
camilamacedo86
force-pushed
the
catalog-sync-second-poc
branch
from
44d12e9 to
963c77f
Compare
|
@camilamacedo86: This pull request references OPRUN-3795 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.19.0" version, but no target version was set. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
@camilamacedo86: This pull request references OPRUN-3795 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.19.0" version, but no target version was set. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
camilamacedo86
force-pushed
the
catalog-sync-second-poc
branch
2 times, most recently
from
eaea787 to
680d4cf
Compare
| // Required to allow run the tests on Mac | ||
| sysCtx := &types.SystemContext{ | ||
| OSChoice: "linux", | ||
| ArchitectureChoice: "amd64", | ||
| } |
There was a problem hiding this comment.
Pre-disposing containers/image to a particular OS and arch will interfere with our ability to evaluate the image at the OCI level.
We want to actually evaluate the root reference. We don't want containers/image automatically de-referencing a manifest list/index and giving us just one of the underlying manifests.
This reminds me: for each root reference, we:
- expect it to be a manifest list or index (I'm not sure which, we should figure that out and then require either manifest list or index, not either).
- will have N filesystem and FBC checks to run, where N is the number of manifests in the manifest list/index.
There was a problem hiding this comment.
I think we’re still able to handle manifest lists—we can loop through the items and run checks as expected.
This change was mostly to make tests work on Mac. Without setting the OS, I was getting this error:
err: <*errors.errorString>{
s: "no image found in manifest list for architecture \"arm64\", variant \"v8\", OS \"darwin\"", <== see looking for darwin :-(
}
I cleaned it up a bit and added a comment. Hope that’s okay!
Thanks!
camilamacedo86
force-pushed
the
catalog-sync-second-poc
branch
3 times, most recently
from
0775f31 to
0185984
Compare
| // } | ||
| sysCtx := &types.SystemContext{ | ||
| OSChoice: "linux", | ||
| } |
There was a problem hiding this comment.
@joelanford ^ See the comment and update to clarify it better.
|
@camilamacedo86: This pull request references OPRUN-3795 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.19.0" version, but no target version was set. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
| var images = []string{ | ||
| "registry.redhat.io/redhat/community-operator-index:v4.18", | ||
| "registry.redhat.io/redhat/redhat-marketplace-index:v4.18", | ||
| "registry.redhat.io/redhat/certified-operator-index:v4.18", | ||
| "registry.redhat.io/redhat/redhat-operator-index:v4.18", | ||
| } |
There was a problem hiding this comment.
For now it is okay for these to be hardcoded, but let's change these to 4.19 tags. We are technically now in 4.20 development window, but I don't think 4.20 catalogs have been bootstrapped.
As soon as this merges, we should open a PR to bump these to 4.20 (which will fail), and then we go hound the pipelines to get the 4.20 bootstrapping done.
As a follow-up to this PR, we should avoid maintaining this list separately. We already have a set of default catalogs in our openshift manifests, so it would be nice to derive this list from our existing default ClusterCatalog objects.
There was a problem hiding this comment.
OK. I updated to 4.19 and we can track this need.
We do that in a follow up.
|
|
||
| var _ = Describe("Catalog Image Validation Suite", func() { | ||
| for _, url := range images { | ||
| name := getImageNameFrom(url) |
There was a problem hiding this comment.
Is there some reason we need to do the name remapping? It could be useful if the entire ref shows up in the test name.
There was a problem hiding this comment.
We are showing the name of the image
Do we need more than that?
I think the full URL is too long
WDYT
openshift/catalog-sync/Makefile
Outdated
| @@ -0,0 +1,3 @@ | |||
| .PHONY: test-catalog-sync | |||
There was a problem hiding this comment.
What's the reasoning behind adding a whole new Makefile? Does it not make sense to just add this to openshift/Makefile? It would make it easier to run the commands through the openshift/release CI if they're all using the same Makefile.
There was a problem hiding this comment.
These tests aren't directly related to OLMv1 components or its release.
The idea behind placing them under the catalog-sync directory was to keep everything related to those tests self-contained. That way, if we ever need to move or remove them in the future, it’ll be easier to manage without impacting other parts of the project. It also helps with maintainability and clarity.
There was a problem hiding this comment.
On top of that, we might need to add more targets in the future.
camilamacedo86
force-pushed
the
catalog-sync-second-poc
branch
6 times, most recently
from
efd4a53 to
bf6e08e
Compare
| CatalogHasValidMetadata(), | ||
| CatalogHasValidMetadataForChannelHeads(), |
There was a problem hiding this comment.
Sorry, I think we flew past each other in yesterday's review I left in this area.
The metadata check should be:
- All channel heads have
olm.csv.metadata - Nothing has
olm.bundle.object
We have no opinion (yet) about whether non-channel heads have olm.csv.metadata
There was a problem hiding this comment.
Hi @joelanford
OK. I am changing for that. However, that does not seems the check that we have in : https://github.com/konflux-ci/build-definitions/blob/main/task/fbc-validation/0.1/fbc-validation.yaml#L236-L253
(Code shared ^ when we did our first meeting) I mean, looking the code above I understand that we should check
var hasCSV, hasObject bool
for _, prop := range bundle.Properties {
if prop.Type == "olm.csv.metadata" {
hasCSV = true
}
if prop.Type == "olm.bundle.object" {
hasObject = true
}
}
if (hasCSV && hasObject) || (!hasCSV && !hasObject) {
errs = append(errs, fmt.Errorf("bundle %q in package %q has both or "+
"neither of olm.bundle.object / olm.csv.metadata", bundle.Name, bundle.Package))
}
Instead of ensuring that all bundles do not have :
if prop.Type == "olm.bundle.object" {
hasObject = true
}
There was a problem hiding this comment.
Neverless it is done as requested now 👍 So, addressed.
camilamacedo86
force-pushed
the
catalog-sync-second-poc
branch
from
bf6e08e to
b88bfe4
Compare
|
@camilamacedo86: This pull request references OPRUN-3795 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.19.0" version, but no target version was set. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
camilamacedo86
force-pushed
the
catalog-sync-second-poc
branch
from
b88bfe4 to
354800a
Compare
camilamacedo86
force-pushed
the
catalog-sync-second-poc
branch
from
354800a to
dbf3a57
Compare
camilamacedo86
changed the title
|
/test openshift-e2e-aws |
|
@camilamacedo86: The following tests failed, say
Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
This PR introduces the initial implementation to allow us to begin testing the catalogues.
The following is the output of its execution.
latest_results.txt
c/c @grokspawn @dtfranz @joelanford