Conversation
…se.openshift.io/delete=true
openshift-ci
bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
|
Important Review skippedAuto reviews are limited based on label configuration. 🚫 Excluded labels (none allowed) (1)
Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: hongkailiu 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 |
openshift-ci
bot
added
the
approved
|
/test all |
|
/retest-required |
hongkailiu
force-pushed
the
pr1309
branch
4 times, most recently
from
eb34c34 to
5073c9b
Compare
|
/test all |
|
/test e2e-agnostic-ovn |
|
@hongkailiu: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions 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. |
|
The factory method is pretty good, could you merge it? I want to keep my pr clean. |
| o.Expect(err).NotTo(o.HaveOccurred()) | ||
| g.By(fmt.Sprintf("Checking if getting manifests with %s on the cluster led to not-found error", annotation)) | ||
|
|
||
| ignore := sets.New[string]("release-metadata") |
There was a problem hiding this comment.
Adding one more file name:
ignore := sets.New[string]("release-metadata", "image-references")
There was a problem hiding this comment.
Is it possible to have the annotation on "image-references"?
Moreover, would it fail on it without being ignored?
There was a problem hiding this comment.
Both of them are not manifest files, so we'd better to skip them before manifest.ManifestsFromFiles([]string{filePath})
There was a problem hiding this comment.
$ cat 419/image-references | head -n 11
{
"kind": "ImageStream",
"apiVersion": "image.openshift.io/v1",
"metadata": {
"name": "4.19.1",
"creationTimestamp": "2025-06-19T15:00:35Z",
"annotations": {
"release.openshift.io/from-image-stream": "ocp/4.19-art-assembly-4.19.1"
}
},
"spec": {
It is a json file instead of yaml like others.
Seems this file is special. It does not even have a namespace for the IS. Probably not applied to the cluster.
| o.Expect(err).NotTo(o.HaveOccurred()) | ||
| _, err = client.Get(ctx, ms.Obj.GetName(), metav1.GetOptions{}) | ||
| o.Expect(apierrors.IsNotFound(err)).To(o.BeTrue(), | ||
| fmt.Sprintf("The deleted manifest should not be installed, but actually installed: manifest: %s %s in namespace %s from file %q, error: %v", |
There was a problem hiding this comment.
It seems the output going to be too detailed, do we really need all of them? I think the message can help us find the installed manifest is enough.
There was a problem hiding this comment.
It will help to tell which object. One file could contain several objects.
When you try with a failure case, you will see.
string(ms.Raw) is more verbose than the current one.
|
|
||
| for _, ms := range manifests { | ||
| ann := ms.Obj.GetAnnotations() | ||
| if ann[annotation] != "true" { |
There was a problem hiding this comment.
Will it cause unexpected issue when we remove ann == nil?
There was a problem hiding this comment.
GoLang does not need nil checking before reading the a map.
2 participants
No description provided.