project & starterproject validation bug

[yangcao77]

What does this PR do?

This PR fixes the bug in project & starterproject validation. detailed description in #339

What issues does this PR fix or reference?

#339

Is your PR tested? Consider putting some instruction how to test your changes

Added more unit test cases to increase invalid project/starterproject test coverage

Docs PR

[amisevsk]

This conditional sequence is kind of a nightmare, something like if A || (B && C) then: if A && (!B || (B && !C)) then error, and should be simplified. Maybe something like

switch len(gitSource.Remotes) {
case 0:
  errString += fmt.Sprintf("\nprojects %s should have at least one remote", project.Name)
case 1:
  if gitSource.CheckoutFrom != nil {
    if err := validateRemoteMap(gitSource.Remotes, gitSource.CheckoutFrom.Remote, project.Name); err != nil {
      errString += fmt.Sprintf("\n%s", err.Error())
    }
  }
default: // len(gitSource.Remotes) >= 2
  if gitSource.CheckoutFrom == nil || gitSource.CheckoutFrom.Remote == "" {
    errString += fmt.Sprintf("\nproject %s has more than one remote defined, but has no checkoutfrom remote defined", project.Name)
    continue
  }
  if err := validateRemoteMap(gitSource.Remotes, gitSource.CheckoutFrom.Remote, project.Name); err != nil {
    errString += fmt.Sprintf("\n%s", err.Error())
  }
}

Maybe some of this logic can be moved into validateRemoteMap? That function seems to simple to be broken out since it's just a map access.

[yangcao77]

Thanks for the suggestion. Using a case switch does make the code more readable.
I have also updated the starterProject validation to use case switch as well to be consistent.

[maysunfaisal]

generally looks good, just a small suggestion

[maysunfaisal]

Suggested change

	} else if len(gitSource.Remotes) > 1 || (gitSource.CheckoutFrom != nil && gitSource.CheckoutFrom.Remote != "") {
	if len(gitSource.Remotes) > 1 && (gitSource.CheckoutFrom == nil || (gitSource.CheckoutFrom != nil && gitSource.CheckoutFrom.Remote == "")) {
	errString += fmt.Sprintf("\nproject %s has more than one remote defined, but has no checkoutfrom remote defined", project.Name)
	continue
	}
	} else if len(gitSource.Remotes) > 1 && gitSource.CheckoutFrom != nil {
	err := validateRemoteMap(gitSource.Remotes, gitSource.CheckoutFrom.Remote, project.Name)
	if err != nil {
	errString += fmt.Sprintf("\n%s", err.Error())
	}
	} else if len(gitSource.Remotes) > 1 && gitSource.CheckoutFrom == nil {
	errString += fmt.Sprintf("\nproject %s has more than one remote defined, but has no checkout from remote defined", project.Name)
	}

I would suggest breaking it to different else if conditions. My suggestion handles all the test cases you wrote but if we want to explicitly handle"" checkoutfrom then we can do it the way you suggested,

[amisevsk]

I believe this suggestion doesn't cover

git:
  remotes: ["my-remote"]
  checkoutFrom:
    remote: "another-remote"

but I didn't read too closely.

👍 on splitting this logic up though, it's very complicated.

[amisevsk]

LGTM 👍

[openshift-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: amisevsk, yangcao77
To complete the pull request process, please assign after the PR has been reviewed.
You can assign the PR to them by writing /assign in a comment when ready.

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

Needs approval from an approver in each of these files:

• OWNERS

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