[DPTP-4298] validate _cluster.yaml for sanitized-prow-jobs #4646

pruan-rht · 2025-08-11T22:00:33Z

on a successful run

prow-job-dispatcher -validate-only  --cluster-config-path=$RELEASE_REPO/core-services/sanitize-prow-jobs/_clusters.yaml
{"component":"prow-job-dispatcher","file":"/Users/pruan/workspace/github_repos/dptp_team/ci-tools/cmd/prow-job-dispatcher/main.go:177","func":"main.validateConfigurations","level":"info","msg":"Validating cluster configuration...","severity":"info","time":"2025-08-11T14:31:45-07:00"}

{"component":"prow-job-dispatcher","file":"/Users/pruan/workspace/github_repos/dptp_team/ci-tools/cmd/prow-job-dispatcher/main.go:201","func":"main.validateConfigurations","level":"info","msg":"All validations passed successfully","severity":"info","time":"2025-08-11T14:31:45-07:00"}
{"component":"prow-job-dispatcher","file":"/Users/pruan/workspace/github_repos/dptp_team/ci-tools/cmd/prow-job-dispatcher/main.go:707","func":"main.main","level":"info","msg":"Validation completed successfully","severity":"info","time":"2025-08-11T14:31:45-07:00"}

on a bad config

prow-job-dispatcher -validate-only  --cluster-config-path=$RELEASE_REPO/core-services/sanitize-prow-jobs/_clusters.yaml
{"component":"prow-job-dispatcher","file":"/Users/pruan/workspace/github_repos/dptp_team/ci-tools/cmd/prow-job-dispatcher/main.go:177","func":"main.validateConfigurations","level":"info","msg":"Validating cluster configuration...","severity":"info","time":"2025-08-11T15:00:13-07:00"}
Diff found, exit code: 1
--- /tmp/_clusters.yaml	2025-08-11 15:00:13
+++ /Users/pruan/workspace/github_repos/dptp_team/release/core-services/sanitize-prow-jobs/_clusters.yaml	2025-08-11 14:47:05
@@ -47,6 +47,7 @@
     - gpu
     - highperf
     - rce
+    disabled: true
 gcp:
   - name: build02
     capabilities:

{"component":"prow-job-dispatcher","file":"/Users/pruan/workspace/github_repos/dptp_team/ci-tools/cmd/prow-job-dispatcher/main.go:201","func":"main.validateConfigurations","level":"info","msg":"All validations passed successfully","severity":"info","time":"2025-08-11T15:00:13-07:00"}
{"component":"prow-job-dispatcher","error":"exit status 1","file":"/Users/pruan/workspace/github_repos/dptp_team/ci-tools/cmd/prow-job-dispatcher/main.go:704","func":"main.main","level":"fatal","msg":"Validation failed","severity":"fatal","time":"2025-08-11T15:00:13-07:00"}

openshift-ci · 2025-08-11T22:00:57Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: pruan-rht

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~~ [pruan-rht]

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

Prucek

Could you also add some tests?

Prucek · 2025-08-18T11:47:06Z

cmd/prow-job-dispatcher/main.go

 	"flag"
 	"fmt"
 	"io/fs"
+	"log"


Why add log, when we have logrus?

I was just using that for less cluter print out, I've taken it out.

Prucek · 2025-08-18T11:51:58Z

cmd/prow-job-dispatcher/main.go

 	}

+	// If validate-only mode is enabled, run validation and exit
+	if o.validateOnly {


I would add this to the o.validate method

good call, done

Prucek · 2025-08-18T11:58:06Z

pkg/dispatcher/helpers.go

+		return err
+	}
+
+	// Parse the original YAML structure


you have the loadClusterConfigFromBytes function already

that function didn't work correctly for me when I tried it. That's why I started the SaveClusterCOnfigPreservingFormat()

I would still like to use the original structure, instead of creating a new one. Once there is a change in there, this change will be forgotten, and all validation will fail.

Prucek · 2025-08-18T12:03:37Z

pkg/dispatcher/helpers.go

+
+// SaveClusterConfigPreservingFormat saves the ClusterMap to YAML while preserving the original format, order, and case sensitivity
+// This method reconstructs the YAML with the exact same structure and field order as the original
+func SaveClusterConfigPreservingFormat(clusterMap ClusterMap, blockedClusters sets.Set[string], originalFilePath, outputFilePath string) error {


Could you refactor this function? IMO it is a bit too long and it could be separated into separate functions that can be unit-tested.

done and added a some tests in the release repo

refactored large function into smaller chunks and fold `validate_only` into `validate()`

openshift-ci · 2025-08-21T04:18:03Z

@pruan-rht: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/security	`e92e6d5`	link	false	`/test security`
ci/prow/breaking-changes	`e92e6d5`	link	false	`/test breaking-changes`

Full PR test history. Your PR dashboard.

Instructions 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.

Prucek · 2025-08-25T10:00:57Z

Can we somehow test the available capabilities? Do we know what capabilities exist?

Prucek · 2025-08-25T09:57:21Z

cmd/prow-job-dispatcher/main.go

-	return o.PrometheusOptions.Validate()
+
+	// Prometheus validation is not required in validate-only mode
+	if !o.validateOnly {


You have this condition in the function three times. I think if put correctly, it can be there only once

Prucek · 2025-08-25T10:07:14Z

cmd/prow-job-dispatcher/main.go

+			}
+		}
+
+		fmt.Println(string(output)) // Print the diff output


Could you please remove some of those comments? In some places, I would agree with them. But they are mostly disturbing. The code should be sufficient most of the time

Prucek · 2025-08-25T10:15:22Z

cmd/prow-job-dispatcher/main_test.go

+		clusterConfig  string
+		wantErr        bool
+		expectedErrMsg string
+		allowDiffError bool // Allow diff errors as they are informational


Could you explain this to me? I would assume if I diff the file, every diff error is relevant. Why not?

Prucek · 2025-08-25T10:21:19Z

cmd/prow-job-dispatcher/main_test.go

+			expectedErrMsg: "failed to load config",
+			allowDiffError: false,
+		},
+	}


Could you maybe add more tests, with all sorts of different errors? They don't need to be files; the struct can be created in the test itself.
I would like to see tests:

if there is a capacity outside of the range 0-100

indentation errors

blocked with capacity/ blocked with disabled? (I don't know what is allowed)
...
try to cover most of the common mistakes that can occur

[DPTP-4298] validate _cluster.yaml for sanitized-prow-jobs

57a4b2b

openshift-ci bot requested review from Prucek and bear-redhat August 11, 2025 22:00

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 11, 2025

pruan-rht mentioned this pull request Aug 11, 2025

[DPTP-4298] add supports for _cluster.yaml validation openshift/release#68032

Closed

Prucek reviewed Aug 18, 2025

View reviewed changes

address comments

e92e6d5

refactored large function into smaller chunks and fold `validate_only` into `validate()`

Prucek reviewed Aug 25, 2025

View reviewed changes

[DPTP-4298] validate _cluster.yaml for sanitized-prow-jobs #4646

Are you sure you want to change the base?

[DPTP-4298] validate _cluster.yaml for sanitized-prow-jobs #4646

Uh oh!

Conversation

pruan-rht commented Aug 11, 2025

Uh oh!

openshift-ci bot commented Aug 11, 2025

Uh oh!

Prucek left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

openshift-ci bot commented Aug 21, 2025

Uh oh!

Prucek commented Aug 25, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants