Check files, dir and mountpoint customizations against our rules from the images library (RHINENG-20656)

[schuellerf]

For direct feedback before creating or updating a blueprint,
we'll do basic checks that will fail later on, anyway.

Needed for faster feedback in direct API usage or MCP server usage.

The original problem was a blueprint with a wrong (not-"allowed") file-path (in the file customization)
was accepted and led to errors later in the process.

Ref

[avitova]

Love the code coverage.

[schuellerf]

@avitova @lzap Thanks for your great & detailed reviews!
I moved the checks to a separate file now and renamed all to "rules" as "validation" (e.g. against json spec) is not done here. Just to distinguish this behavior.
Also I implemented the looped execution of the checks including the behavior to return all problems at once (instead of only the first problem)
Please check if this is better now

[lzap]

Looks great, the code can be greatly simplified tho.

[avitova]

Looks good. I have one nitpick, but it looks good.

[schuellerf]

@avitova @lzap Thanks a lot for all your valuable input, I think I addressed all your comments, please check

[lzap]

The code looks good, tho, I am unable to locate where ErrBlueprintValidation value is declared.

[croissanne]

I wonder if it makes sense to try to move towards https://github.com/osbuild/blueprint as the internal representation, as we're effectively doubling all of this right now ig. Not something for this PR however.

Could you clean u pthe commit history a little though, some suggestions:

• the change to the generated files are just because of a differnet version of oapi-codegen? Not sure why this is part of this PR, i think this commit can be dropped?
• "simplify error handling" isn't very descriptive, in what way is it simplified?
• "initially I wanted..." -> could we reword this to be a bit more neutral like, it should state what it's changing and why. Just like "split out validation of blueprint into separate functions, one per customization, to make the code more manageable/readable" or something.
• I think it mkaes sense to move the rework and split to the top, as the first commit, and then add the file validations after (will make the history a bit more readable too).
• "blueprint_rules: remove references to a validation" -> I'd just squash this one / move it to the top

[lzap]

Absolutely, if we move towards the blueprint repo now then it will make change to UBP much easier as we will be able to deprecate the API and use the UBP convertor and V2 in parallel easily. Having said that, I suggest starting with composer first, then doing it here.

I agree that commits must be squashed before merging, I think @schuellerf just wants to make review process easier which I really appreciate. But in the end, we all want a clean git history.

[schuellerf]

@croissanne

the change to the generated files are just because of a differnet version of oapi-codegen? Not sure why this is part of this PR, i think this commit can be dropped?

This was needed to get the checks green… (maybe waiting/splitting off and re-basing on this change also helps …)

@croissanne @lzap I would love to rework / squash and force push all commits but many people in the team say, they like to review in-order… often when iteratively working on a change I can't simply reorder but need to squash all in the end if the strategy of implementation changed within the review (which is fine, but either keep the commits or squash all )

[schuellerf]

@lzap

I agree that commits must be squashed before merging, I think @schuellerf just wants to make review process easier which I really appreciate. But in the end, we all want a clean git history.

Exactly… How do I decide when it's "the end" and I should squash them?
Just before merging, when all is approved? Then you need to re-approve and (somehow?) check that I didn't make a mistake in squashing …which I never do, obviously ;)

[croissanne]

Well I'm good with merging this after the history is cleaned up a bit / rebased a bit. I guess it's tricky, but I think when most issues in the code have been addressed is a good time.

[schuellerf]

The iterative commits couldn't really be sorted easily - maybe squashing it all into one is better :-/

[lzap]

Just for the record, by "squashing" I did not mean to squash everything to a single commit. What I meant was to squash "fix" commits into appropriate places. This is not a big PR so I think keeping it in just two commits is fine with me.

[lzap]

One additional request, can you add a test for that error conversion function? It is a bit complex. Here is one that works (if you add a check for nil):

package main

import (
	"errors"
	"reflect"
	"testing"
)

func TestToHTTPErrorList(t *testing.T) {
	testCases := []struct {
		name     string
		err      error
		expected HTTPErrorList
	}{
		{
			name: "single validationError",
			err:  validationError{title: "t1", detail: "d1"},
			expected: HTTPErrorList{
				Errors: []HTTPError{
					{Title: "t1", Detail: "d1"},
				},
			},
		},
		{
			name: "joined validationErrors",
			err: errors.Join(
				validationError{title: "t1", detail: "d1"},
				validationError{title: "t2", detail: "d2"},
			),
			expected: HTTPErrorList{
				Errors: []HTTPError{
					{Title: "t1", Detail: "d1"},
					{Title: "t2", Detail: "d2"},
				},
			},
		},
		{
			name: "joined validationError and other error",
			err: errors.Join(
				validationError{title: "t1", detail: "d1"},
				errors.New("some other error"),
			),
			expected: HTTPErrorList{
				Errors: []HTTPError{
					{Title: "t1", Detail: "d1"},
					{Title: "validation error", Detail: "some other error"},
				},
			},
		},
		{
			name: "single other error",
			err:  errors.New("some other error"),
			expected: HTTPErrorList{
				Errors: []HTTPError{
					{Title: "validation error", Detail: "some other error"},
				},
			},
		},
		{
			name: "nil error",
			err:  nil,
			expected: HTTPErrorList{
				Errors: nil,
			},
		},
		{
			name: "nested joined errors",
			err: errors.Join(
				validationError{title: "t1", detail: "d1"},
				errors.Join(
					validationError{title: "t2", detail: "d2"},
					errors.New("some other error"),
				),
			),
			expected: HTTPErrorList{
				Errors: []HTTPError{
					{Title: "t1", Detail: "d1"},
					{Title: "t2", Detail: "d2"},
					{Title: "validation error", Detail: "some other error"},
				},
			},
		},
	}

	for _, tc := range testCases {
		t.Run(tc.name, func(t *testing.T) {
			actual := toHTTPErrorList(tc.err)
			if !reflect.DeepEqual(actual, tc.expected) {
				t.Errorf("expected %+v, got %+v", tc.expected, actual)
			}
		})
	}
}

[lzap]

You can drop this type assertion, the errors.As will cover that case.

[avitova]

I am good here, and do not know why this still shows requested changes from me. 👀 Let's try if this helps.

It still shows requested changes, github plz

[lzap]

I really think that internal/v1/handler_blueprints_internal_test.go (and the code logic) belongs to a different package. But this is acceptable.

Please rebase and run the prepare-source.sh once again to fix CI :-)

[avitova]

TY:)

[lzap]

Oh well, prepare source...