Fix nightly tests and refactor add tests

[rokshana-b]

What changed?

• running nightly tests 6 days a week to leave Sunday free for the new upcoming weekly tests
• refactor add tests to prevent nightly failures

How did you test it?

• ran nightly on ci, it passed

[josecordaz]

I wonder if we need else cases in all the places we have this validation. I ask because there is an else in line 1728.

[rokshana-b]

We need this validation when we run tests on gitlab repos; this validation fails for gitlab

[josecordaz]

So we are not going to cover the gitlab case in this moment?

[rokshana-b]

Context:

This PR was originally opened to enable test runs in gitlab repos. However, the tests couldn't be run on parallel since we have standing clusters and that would create conflict(and failures). So I tried running the tests one after another. First the acceptance tests would run on github, then on gitlab. This worked but it took ridiculously long to run: https://github.com/weaveworks/weave-gitops/actions/runs/1721328971 (almost 4 hrs!!)
Nightly tests are famously hard to fix because of the long runtime and adding more time is not ideal.

Fix:

So, for now, I've refactored stuff to get the nightly passing and I've configured the nightly to run Mon-Sat. I have a plan to write up another job that will run every week on Sun that will cover the entire github/gitlab workflow. This seemed like a reasonable bargain — run time-consuming tests weekly instead of daily.

To answer your original question — no, we don't need an else for all the if conditions. Because for line 1728, we have different outcomes depending on github/gitlab repos and both outcomes need to be verified. For RepoVisibility check, however, we only need this to work when GIT_PROVIDER is NOT gitlab. Otherwise, it will just run as is. And the default behavior pretty much in every workflow is to use github.

@josecordaz Hope this answers your question

[josecordaz]

@rokshana-b Sorry probably I'm not making myself understood. It's just that it seems weird to me that we have a By clause saying And repo created has private visibility, and when I see this if os.Getenv("GIT_PROVIDER") != "gitlab" { it tells that we are not going to check the visibility of the repo for gitlab. But what is still not clear to me is "why". You mentioned we only need this to work when GIT_PROVIDER is NOT gitlab but this doesn't explain why. What is even stranger for me is that we are using go-git-providers in function getGitRepoVisibility which in theory should handle both types of providers. At this point I wonder if the "why" is a bug in the go-git-providers repo. If this fails for gitlab what would be the error?

[rokshana-b]

@josecordaz I wanted to leave a msg for you in this PR so its documented. There's no real bug here. The problem is for github, the repo visibility is either private or public — simple as that. For gitlab, however, the repo visibility depends on the group. So, everytime we create a private repo for github, it should work for gitlab. But if we create a public repo for github, the same will not be true for gitlab. If the gitlab-repo-group is private, the repo visibility will remain private.

The failures I saw were related to github being a public repo and gitlab remaining as private. It was probably overkill for me to disable repo visibility checks for all gitlab tests. I will re-add the checks wherever the github repos are private in another PR.

[josecordaz]

@rokshana-b I see you removed env var K8S_VERSION from some config files. Do we need to rewrite it? or is it present already?

func getK8sVersion() string {
	k8sVersion, found := os.LookupEnv("K8S_VERSION")
	if found {
		return k8sVersion
	}

	return DEFAULT_K8S_VERSION
}

[rokshana-b]

@rokshana-b I see you removed env var K8S_VERSION from some config files. Do we need to rewrite it? or is it present already?

func getK8sVersion() string {
	k8sVersion, found := os.LookupEnv("K8S_VERSION")
	if found {
		return k8sVersion
	}

	return DEFAULT_K8S_VERSION
}

@josecordaz I removed the K8S version from nightly because we the clusters have fixed versions and they need to be changed manually from the console. So we don't need to re-add it