feat: refactor manifest handling #867

cam-garrison · 2024-02-15T16:45:50Z

https://issues.redhat.com/browse/RHOAIENG-3262

Description

The is PR reworks the way that the Feature framework creates and handles manifests. Rather than treat all manifests as one type with flags for how it should be treated, we now define Manifest as an interface, and add the BaseManifest, TemplateManifest, and KustomizeManifest types - adding initial support for Kustomization files. Also, this PR adds the TargetNamespace field to Feature.spec, allowing users to specify a TargetNamespace for their kustomization manifests to be applied to.

BaseManifests are created for processing raw yaml files - no templating needed, just apply yaml to create objects.
TemplateManifests are created for processing .tmpl files - templating is applied to these files (substituting values within file).
Both of these support .patch files.
KustomizeManifests are created for processing kustomization.yaml files (also supports passing in the directory that contains said file).

This allows for better processing of each type of manifest - now the process() method returns a list of unstructured objects to be applied rather than storing strings within the manifest type.

Note that KustomizeManifest's only support files stored within the filesystem - not the embedded filesystem. eg it supports files stored in opt/manifests/....

How Has This Been Tested?

Added new unit tests for each type's Process() method - added the path to this to make unit-test. Also added another feature integration test for a KustomizeManifest.

For testing manually, built image quay.io/cgarriso/opendatahub-operator:dev-split-manifest-types and ensured features within DSCI's service mesh setup still worked as expected. Also, in this image I created some 'fake' features:

createKustTwoErr := feature.CreateFeature("create-kustomization-two").
    For(handler).
    Manifests(path.Join("opt", "manifests", "fake-kust-dir")).
    TargetNamespace("istio-system").
    Load()

ensuring that KustomizeManifest's with a TargetNamespace worked as expected on CRC.

Merge criteria:

The commits are squashed in a cohesive manner and have meaningful messages.
Testing instructions have been added in the PR body (for PRs involving changes that are not immediately obvious).
The developer has manually tested the changes and verified that the changes work

add process unit tests, move apply() logic back to feature add given when then apply and process manifests in one step in Feature.Apply() move feature tracker out of spec add targetNamespace to feature, label+ns setting in kustomize manifest

bartoszmajsak

This very nice improvement. I shared a few suggestions, but other than that solid work!

Thanks for expanding tests!

pkg/feature/builder.go

pkg/feature/manifest.go

pkg/feature/feature.go

pkg/feature/manifest.go

pkg/feature/raw_resources.go

Co-authored-by: Bartosz Majsak <bartosz.majsak@gmail.com>

let k8s handle it

cam-garrison · 2024-02-21T18:52:05Z

@bartoszmajsak re-tested e2e on crc after resolving merge conflicts + addressing review. Re-requesting review now when you have time 😃

bartoszmajsak

Few small nits, last round :)

pkg/feature/manifest.go

pkg/feature/manifest_test.go

pkg/feature/raw_resources.go

Co-authored-by: Bartosz Majsak <bartosz.majsak@gmail.com>

cam-garrison · 2024-02-26T20:16:55Z

Hi @zdtsw, removed the refactoring label as I've addressed Bartosz's review. Please take a look whenever you have the chance. Thanks a ton!

zdtsw · 2024-03-02T08:09:19Z

Hi @zdtsw, removed the refactoring label as I've addressed Bartosz's review. Please take a look whenever you have the chance. Thanks a ton!

sorry, did not get back to you asap.

looks good for me and thanks for the hard (rebase) work

bartoszmajsak

lgtm

would be good to merge incubation back so the tests will run and then squash merge PR.

openshift-ci · 2024-03-04T18:33:51Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bartoszmajsak, zdtsw

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [zdtsw]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-ci · 2024-03-06T07:17:09Z

New changes are detected. LGTM label has been removed.

zdtsw · 2024-03-06T08:58:14Z

/test opendatahub-operator-e2e

Co-authored-by: Bartosz Majsak <bartosz.majsak@gmail.com> Reworks the way that the `Feature` framework creates and handles manifests. Rather than treat all manifests as one type with flags for how it should be treated, we now define Manifest as an interface, and add the `BaseManifest`, `TemplateManifest`, and `KustomizeManifest` types - adding initial support for Kustomization files. Also, this PR adds the `TargetNamespace` field to Feature.spec, allowing users to specify a TargetNamespace for their kustomization manifests to be applied to. - `BaseManifest`s are created for processing raw yaml files - no templating needed, just apply yaml to create objects. - `TemplateManifest`s are created for processing .tmpl files - templating is applied to these files (substituting values within file). Both of these support .patch files. - `KustomizeManifest`s are created for processing `kustomization.yaml` files (also supports passing in the directory that contains said file). This allows for better processing of each type of manifest - now the process() method returns a list of unstructured objects to be applied rather than storing strings within the manifest type. Note that `KustomizeManifest`'s only support files stored within the filesystem - not the embedded filesystem. e.g. it supports files stored in `/opt/manifests/...`.

Co-authored-by: Bartosz Majsak <bartosz.majsak@gmail.com> Reworks the way that the `Feature` framework creates and handles manifests. Rather than treat all manifests as one type with flags for how it should be treated, we now define Manifest as an interface, and add the `BaseManifest`, `TemplateManifest`, and `KustomizeManifest` types - adding initial support for Kustomization files. Also, this PR adds the `TargetNamespace` field to Feature.spec, allowing users to specify a TargetNamespace for their kustomization manifests to be applied to. - `BaseManifest`s are created for processing raw yaml files - no templating needed, just apply yaml to create objects. - `TemplateManifest`s are created for processing .tmpl files - templating is applied to these files (substituting values within file). Both of these support .patch files. - `KustomizeManifest`s are created for processing `kustomization.yaml` files (also supports passing in the directory that contains said file). This allows for better processing of each type of manifest - now the process() method returns a list of unstructured objects to be applied rather than storing strings within the manifest type. Note that `KustomizeManifest`'s only support files stored within the filesystem - not the embedded filesystem. e.g. it supports files stored in `/opt/manifests/...`. (cherry picked from commit ec81ef6)

cam-garrison added 2 commits February 15, 2024 10:43

use ginkgo v2 for manifest unit test

e67774c

cam-garrison requested review from bartoszmajsak and zdtsw February 15, 2024 16:45

openshift-ci bot requested review from etirelli and VaishnaviHire February 15, 2024 16:47

bartoszmajsak mentioned this pull request Feb 15, 2024

feat(authz): Authorino for Service Mesh #784

Merged

3 tasks

bartoszmajsak requested changes Feb 15, 2024

View reviewed changes

openshift-ci bot assigned bartoszmajsak Feb 15, 2024

cam-garrison and others added 15 commits February 15, 2024 16:01

Update pkg/feature/manifest.go

70d90a3

Co-authored-by: Bartosz Majsak <bartosz.majsak@gmail.com>

replace interface{} with any

40b008c

Co-authored-by: Bartosz Majsak <bartosz.majsak@gmail.com>

fix comments + assertions

e74983a

Co-authored-by: Bartosz Majsak <bartosz.majsak@gmail.com>

suggestions for TargetNamespace

647da26

Co-authored-by: Bartosz Majsak <bartosz.majsak@gmail.com>

fix err declaration, set target ns in load

dabdca7

review: simplify process

37d9d6e

Co-authored-by: Bartosz Majsak <bartosz.majsak@gmail.com>

move contracts + de-export const

3da0ad5

extract to processManifests

9b36eea

Update pkg/feature/manifest.go

3c584f3

Co-authored-by: Bartosz Majsak <bartosz.majsak@gmail.com>

address remaining comments

6b72743

chore: removes nil check for fsys

e3672ee

chore: minor cleanups

ca64faa

chore: unexports ownerref metaoption

c964e54

chore: minor test rename

598697e

chore: do not assume anything about ns in manifests

3c0a70c

let k8s handle it

zdtsw added the refactoring label Feb 16, 2024

remove need for isPatch()

7b10cb1

openshift-merge-robot added the needs-rebase label Feb 20, 2024

Merge branch 'incubation' into refactor-manifest-handling

a94a1ce

openshift-merge-robot removed the needs-rebase label Feb 20, 2024

fix linter, restore f.ApplyManifest()

9fd808e

cam-garrison added 2 commits February 20, 2024 15:09

Merge branch 'incubation' into refactor-manifest-handling

1aa6be3

remove kustomization todo

8613e6c

cam-garrison requested a review from bartoszmajsak February 21, 2024 18:52

bartoszmajsak requested changes Feb 22, 2024

View reviewed changes

cam-garrison and others added 4 commits February 22, 2024 15:37

apply comment/error suggestions

5ed2963

Co-authored-by: Bartosz Majsak <bartosz.majsak@gmail.com>

Update pkg/feature/raw_resources.go

0dd409c

Co-authored-by: Bartosz Majsak <bartosz.majsak@gmail.com>

final review to test+err

3588b85

Merge branch 'incubation' into refactor-manifest-handling

7376b8d

cam-garrison removed the refactoring label Feb 26, 2024

zdtsw approved these changes Mar 2, 2024

View reviewed changes

openshift-ci bot assigned zdtsw Mar 2, 2024

openshift-ci bot added lgtm approved labels Mar 2, 2024

zdtsw added the odh-2.9 label Mar 2, 2024

bartoszmajsak approved these changes Mar 4, 2024

View reviewed changes

bartoszmajsak added the refactoring label Mar 6, 2024

Merge branch 'incubation' into refactor-manifest-handling

2666628

openshift-ci bot removed the lgtm label Mar 6, 2024

bartoszmajsak enabled auto-merge (squash) March 6, 2024 07:18

Merge branch 'incubation' into refactor-manifest-handling

e043aaf

bartoszmajsak merged commit 86c1695 into opendatahub-io:incubation Mar 6, 2024
6 of 7 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: refactor manifest handling #867

feat: refactor manifest handling #867

cam-garrison commented Feb 15, 2024 •

edited

bartoszmajsak left a comment

cam-garrison commented Feb 21, 2024

bartoszmajsak left a comment

cam-garrison commented Feb 26, 2024

zdtsw commented Mar 2, 2024

bartoszmajsak left a comment

openshift-ci bot commented Mar 4, 2024

openshift-ci bot commented Mar 6, 2024

zdtsw commented Mar 6, 2024

feat: refactor manifest handling #867

feat: refactor manifest handling #867

Conversation

cam-garrison commented Feb 15, 2024 • edited

Description

How Has This Been Tested?

Merge criteria:

bartoszmajsak left a comment

Choose a reason for hiding this comment

cam-garrison commented Feb 21, 2024

bartoszmajsak left a comment

Choose a reason for hiding this comment

cam-garrison commented Feb 26, 2024

zdtsw commented Mar 2, 2024

bartoszmajsak left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Mar 4, 2024

openshift-ci bot commented Mar 6, 2024

zdtsw commented Mar 6, 2024

cam-garrison commented Feb 15, 2024 •

edited