crdutil: Add feature to also apply single files #58

tobiasgiese · 2024-11-25T09:23:48Z

This PR adds a feature to apply single yaml files, not only complete directories.

pkg/crdutil/crdutil.go

coveralls · 2024-11-25T09:27:23Z

Pull Request Test Coverage Report for Build 12053812919

Details

10 of 45 (22.22%) changed or added relevant lines in 1 file are covered.
2 unchanged lines in 1 file lost coverage.
Overall coverage decreased (-0.08%) to 62.059%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
pkg/crdutil/crdutil.go	10	45	22.22%

Files with Coverage Reduction	New Missed Lines	%
pkg/crdutil/crdutil.go	2	37.72%

Totals
Change from base Build 11928826672:	-0.08%
Covered Lines:	1091
Relevant Lines:	1758

💛 - Coveralls

pkg/crdutil/crdutil.go

tobiasgiese · 2024-11-27T00:11:28Z

Tanks for the extensive review @shivamerla. I should have implemented all findings in this diff.

❯ go run cmd/apply-crds/main.go -f /tmp/foo/
2024/11/27 01:25:19 Apply CRDs from file: /tmp/foo/1.yaml
2024/11/27 01:25:19 Update CRD issuers.cert-manager.io

❯ go run cmd/apply-crds/main.go -f /tmp/foo/ -R
2024/11/27 01:25:22 Apply CRDs from file: /tmp/foo/1.yaml
2024/11/27 01:25:22 Update CRD issuers.cert-manager.io
2024/11/27 01:25:22 Apply CRDs from file: /tmp/foo/recursive/2.yaml
2024/11/27 01:25:22 Update CRD orders.acme.cert-manager.io

❯ go run cmd/apply-crds/main.go -f /tmp/foo/1.yaml
2024/11/27 01:25:28 Apply CRDs from file: /tmp/foo/1.yaml
2024/11/27 01:25:28 Update CRD issuers.cert-manager.io

❯ go run cmd/apply-crds/main.go -f /tmp/foo/1.yaml -f /tmp/foo/recursive/2.yaml
2024/11/27 01:25:35 Apply CRDs from file: /tmp/foo/1.yaml
2024/11/27 01:25:35 Update CRD issuers.cert-manager.io
2024/11/27 01:25:35 Apply CRDs from file: /tmp/foo/recursive/2.yaml
2024/11/27 01:25:35 Update CRD orders.acme.cert-manager.io

pkg/crdutil/crdutil.go

vasrem

LGTM overall, just one comment on the linter stuff.

pkg/crdutil/crdutil.go

vasrem · 2024-11-27T08:19:49Z

pkg/crdutil/crdutil.go

+				return nil
+			}
+			// If not recursive we want to only apply the CRDs in the top-level directory.
+			if !recursive && parentDir.IsDir() && filepath.Dir(path) != strings.TrimRight(crdDir, "/") {


should we add a test case for this new branch or should these all be included in the "TODO add unit tests" comment you have added?

Let me take another look at the tests, I think I can easily test the walkCRDs func

pkg/crdutil/crdutil.go

elezar · 2024-11-27T14:25:54Z

pkg/crdutil/crdutil.go

@@ -49,26 +51,32 @@ func (s *StringList) Set(value string) error {
 }

 var (
-	crdsDir StringList
+	files     []string
+	recursive bool


Does it make sense to separate the handling of multiple non-recursive paths (including files) from the addition of recursive handling?

AFAIK kubectl is also not separating it, so it is totally fine to keep it pretty open IMO. But also fine to me to split it somehow (you mean like: if multiple files are passed the recursive flag is not possible?)

I actually just meant as separate PRs. From NVIDIA/gpu-operator#1137 it seems that we don't use recursive mode. Is it used in the network operator?

No, it is not used by the network-operator. Maybe it makes sense to remove this new feature from the PR, you are right. The request adding it to this PR came through #58 (comment) (at least that's how I understood it)

pkg/crdutil/crdutil.go

elezar · 2024-11-27T14:29:31Z

pkg/crdutil/crdutil.go

 )

 func initFlags() {
-	flag.Var(&crdsDir, "crds-dir", "Path to the directory containing the CRD manifests")


Are we concerned about this as a breaking change since the command line arguments are changed?

I was also thinking about this but this pkg is pretty new and we are only using it for the network-operator currently. Do you think we should deprecate it?

If the network-operator is not released with this change, better to just update as it will be first use of this package.

The network-operator 24.10.0 has been released 2 days ago with the change in it, see https://github.com/Mellanox/network-operator/blob/v24.10.0/deployment/network-operator/templates/upgrade-crd-hook.yaml#L84.

But IMO it is totally fine to change it, because it is a Helm pre-install hook. If it fails, it fails the installation/upgrade and the user has to use the latest Helm chart. There is no dependency to the runtime which can cause the operator to fail.

WDYT @e0ne @rollandf

tl;dr: this PR changes the flag --crds-dir to --filename/-f

If there is a concern, we could support both -- marking crds-dir as deprecated -- for a release. Happy with the change since this is effectively an internal API.

We can also revert it to --crds-dir. Both crds-dir and filename does not contain both files and dirs, so it is not 100% explicit anyway :)
Request came through #58 (comment)

@shivamerla WDYT? Should we remove the flag rename and recursive flag (see #58 (comment)) out of this PR and change it in a follow-up? I'd say this will reduce the size of this PR.

pkg/crdutil/crdutil.go

elezar · 2024-11-27T14:32:33Z

pkg/crdutil/crdutil.go

-func walkCrdsDir(ctx context.Context, crdClient v1.CustomResourceDefinitionInterface) error {
-	for _, crdDir := range crdsDir {
+// walkCRDs walks the CRDs directory and applies each YAML file.
+func walkCRDs(ctx context.Context, crdClient v1.CustomResourceDefinitionInterface) error {


As a question: Does it makes sense to separate filename collection from applying the CRDs? That is to say that we would have logic that accepts the flags and returns a slice of YAML file paths. The logic to apply the CRDs would then accept a slice to YAML file paths. This should make things simpler to test and also allow us to properly de-duplicate if a user specifies a file multiple times, for example.

Yes, I think that makes totally sense. Let me change the code accordingly

shivamerla

LGTM! Thanks

pkg/crdutil/crdutil.go

elezar · 2024-11-29T10:41:45Z

pkg/crdutil/crdutil.go

+// walkCRDs walks the CRDs directory and applies each YAML file.
+// TODO: add unit test  for this function.
+func walkCRDs(recursive bool, crdDirs []string) ([]string, error) {


Suggested change

// walkCRDs walks the CRDs directory and applies each YAML file.

// TODO: add unit test for this function.

func walkCRDs(recursive bool, crdDirs []string) ([]string, error) {

// collectYAMLPaths processes a list of paths and returns all YAML files.

// If `recursive` is specified directories in the list are searched recursively for YAML files.

// TODO: add unit test for this function.

func collectYAMLPaths(recursive bool, paths []string) ([]string, error) {

Note that we also need to rename some local variables to better indicate what we are collecting.

pkg/crdutil/crdutil.go

tobiasgiese · 2025-01-10T09:34:50Z

just as a note/reminder: will work on it next week again

Signed-off-by: Tobias Giese <[email protected]>

tobiasgiese · 2025-01-13T11:09:17Z

@elezar @shivamerla I implemented all findings and also removed the --force flag and reverted the flag rename from --filename back to --crds-dir. We can think of deprecating the flag and use --filename in a follow-up PR. Additionally, the force feature can/will be also implemented in a follow-up PR - if needed.

If you both accept the changes I will squash both commits.

…tead Signed-off-by: Tobias Giese <[email protected]>

tariq1890 · 2025-01-22T18:43:12Z

pkg/crdutil/test-files/nested/do-not-apply.yaml

+          type: object
+          properties:
+            spec:
+              type: object


can we add EOL here?

tobiasgiese commented Nov 25, 2024

View reviewed changes