-
Notifications
You must be signed in to change notification settings - Fork 223
NO-JIRA: All CVO manifests in payload should be included #1337
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,7 +1,11 @@ | ||
| package payload | ||
|
|
||
| import ( | ||
| "bytes" | ||
| "fmt" | ||
| "io/fs" | ||
| "os" | ||
| "path/filepath" | ||
| "strings" | ||
| "testing" | ||
|
|
||
|
|
@@ -12,6 +16,7 @@ import ( | |
| "k8s.io/utils/ptr" | ||
|
|
||
| configv1 "github.com/openshift/api/config/v1" | ||
| "github.com/openshift/library-go/pkg/manifest" | ||
| ) | ||
|
|
||
| func TestRenderManifest(t *testing.T) { | ||
|
|
@@ -304,3 +309,86 @@ data: | |
|
|
||
| return yaml | ||
| } | ||
| func Test_cvoManifests(t *testing.T) { | ||
| config := manifestRenderConfig{ | ||
| ReleaseImage: "quay.io/cvo/release:latest", | ||
| ClusterProfile: "some-profile", | ||
| } | ||
|
|
||
| tests := []struct { | ||
| name string | ||
| dir string | ||
| }{ | ||
| { | ||
| name: "install dir", | ||
| dir: filepath.Join("../../install"), | ||
| }, | ||
| { | ||
| name: "bootstrap dir", | ||
| dir: filepath.Join("../../bootstrap"), | ||
| }, | ||
| } | ||
| const prefix = "include.release.openshift.io/" | ||
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| var count int | ||
| err := filepath.WalkDir(tt.dir, func(path string, d fs.DirEntry, err error) error { | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| if d.IsDir() { | ||
| return nil | ||
| } | ||
|
|
||
| var manifestsWithoutIncludeAnnotation []manifest.Manifest | ||
| filePath := filepath.Join(tt.dir, d.Name()) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Here we are joining the
Thus, this would not work for files in sub-directories if such files were present in a recursive search, which is something that the f0e561d commit aims to implement. Please correct me if I am mistaken. I don't mind implementing or not implementing the recursive search; however, I am noting the correctness in this case. |
||
| data, err := os.ReadFile(filePath) | ||
| if err != nil { | ||
| t.Fatalf("failed to read manifest file: %v", err) | ||
| } | ||
| data, err = renderManifest(config, data) | ||
| if err != nil { | ||
| t.Fatalf("failed to render manifest: %v", err) | ||
| } | ||
| manifests, err := manifest.ParseManifests(bytes.NewReader(data)) | ||
| if err != nil { | ||
| t.Fatalf("failed to load manifests: %v", err) | ||
| } | ||
|
|
||
| for _, m := range manifests { | ||
| m.OriginalFilename = filePath | ||
| var found bool | ||
| for k := range m.Obj.GetAnnotations() { | ||
| if strings.HasPrefix(k, prefix) { | ||
| found = true | ||
| break | ||
| } | ||
| } | ||
| if !found { | ||
| manifestsWithoutIncludeAnnotation = append(manifestsWithoutIncludeAnnotation, m) | ||
| } | ||
| } | ||
|
|
||
| if len(manifestsWithoutIncludeAnnotation) > 0 { | ||
| var messages []string | ||
| for _, m := range manifestsWithoutIncludeAnnotation { | ||
| messages = append(messages, fmt.Sprintf("%s/%s/%s/%s", m.OriginalFilename, m.GVK, m.Obj.GetName(), m.Obj.GetNamespace())) | ||
| } | ||
| t.Errorf("Those manifests have no annotation with prefix %q and will not be installed by CVO: %s", prefix, strings.Join(messages, ", ")) | ||
| } | ||
| count++ | ||
| return nil | ||
| }) | ||
|
|
||
| if err != nil { | ||
| t.Fatalf("failed to walk directory: %v", err) | ||
| } | ||
|
|
||
| if count == 0 { | ||
| t.Errorf("no files found in %s", tt.dir) | ||
| } | ||
|
|
||
| }) | ||
| } | ||
| } | ||
Uh oh!
There was an error while loading. Please reload this page.
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.
Noting the 059f3f0 commit message:
The message sounds like we render the
bootstrap-pod.yamlfile, because it is not a valid YAML file. Technically, we render all CVO manifests, which are later applied. Be it CVOinstallmanifests and CVObootstrapmanifests. Specific manifests are rendered during various specific stages:rendercommand, such as the installer (I am not aware of other components using it at the moment).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.
Apologies, wrong function, checking.
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.
Oops, fixed my original comment.