feat(manifest): loader.Load(...) includes unselected environments as skipped with Skip: true

[arthurpitman]

Why this PR?

In order to be able to check environment and group overrides in project.LoadProjects(...), something that will be done in a follow up PR, the loaded manifest needs to contain at least the names of all environment and groups, and not just those selected to be deployed or deleted.

What has changed?

An additional field, Skip is added to manifest.EnvironmentDefinition:

• With Skip set to false, the environment should be used as previously; it is not skipped and therefore is selected for deployment or deletion.
• With Skip set to true, the environment is loaded within its group, but is not selected for deployment or deletion. Such an environment won't have its URL or credentials resolved.

How does it do it?

• loader.Load(...) now loads all environments defined in the manifest, setting the Skip field appropriately.
• Small changes are made to dynatrace.VerifyEnvironmentGeneration(...), dynatrace.CreateEnvironmentClients(...), project.toEnvironmentSlice(...) to make them skip non-Enabled environments.

How is it tested?

• Existing tests are updated, predominantly to add the Skip: true for environments that should be skipped.
• Two new tests are added: TestLoadConfigFile_ConfigsGeneratedForNonSkippedEnvironments and TestLoadConfigFile_NoConfigsGeneratedForSkippedEnvironments

How does it affect users?

It has no effect on users.

[github-actions]

E2E Test Results

    3 files  ±0    261 suites  ±0   23m 26s ⏱️ + 1m 32s
2 156 tests ±0  2 154 ✅ +2  2 💤 ±0  0 ❌  - 2 
2 363 runs  ±0  2 361 ✅ +2  2 💤 ±0  0 ❌  - 2 

Results for commit 788ac03. ± Comparison against base commit 8c61c24.

♻️ This comment has been updated with latest results.

[arthurpitman]

I'm pretty satisfied that E2E tests are working with this change, although I've been unable to get them all green in one run here.

[Kirdock]

At the moment, we have a small behavior change here
https://github.com/Dynatrace/dynatrace-configuration-as-code/pull/1876/files#diff-f0178ccba6ea85e4b333a9ba0913163f69960f5361186b218cab7b25d5936d1cR188
If all definitions are skipped, it resulted in an error before.

Here is also a behavior change, right?

dynatrace-configuration-as-code/cmd/monaco/deploy/deploy.go

Line 159 in 485b408

undefinedEnvironments[envName] = struct{}{}

Previously, it was marked as an undefined environment

Not sure about this though

dynatrace-configuration-as-code/pkg/project/project_loader.go

Line 123 in 485b408

projectNamesToLoad = append(projectNamesToLoad, project.Dependencies[environment.Name]...)

Anyway, it would be nice if @Laubi or @tomazpu could also have a look, as this touches a lot of places :)

[Kirdock]

[nit]: When looking at this, Skip would fit a bit better. We're using Skip for configs, and the condition and logs for excluding this environment are also "skip".

[arthurpitman]

So Skip: true would mean "don't use this environment"? I like that idea 👍

[arthurpitman]

Done 👍

[arthurpitman]

@Kirdock, you're correct: we need to clarify what the expected behavior is. The code:

dynatrace-configuration-as-code/cmd/monaco/deploy/deploy.go

Lines 157 to 161 in 485b408

	_, found := envs[envName]
	if !found {
	undefinedEnvironments[envName] = struct{}{}
	continue
	}

doesn't really work as configs are only created for environments in the loaded manifest, which means no configs will violate this. There is even a test for this:

dynatrace-configuration-as-code/cmd/monaco/deploy/deploy_test.go

Lines 166 to 181 in 485b408

	t.Run("undefined environment in project fails", func(t *testing.T) {
	err := validateProjectsWithEnvironments(
	t.Context(),
	[]project.Project{
	{
	Id: project1Id,
	Configs: project.ConfigsPerTypePerEnvironments{
	"unknown_env": project.ConfigsPerType{},
	},
	},
	},
	manifest.Environments{
	env1Id: env1Definition,
	})
	assert.ErrorContains(t, err, "undefined environment")
	})

[arthurpitman]

In response to sonarqubecloud, I'll increase test coverage, particularly around loader.LoadConfigFile(...).

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
100.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[Laubi]

We now have a lot of these env.Skip checks (7), and I am thinking if it would be possible to not even pass them into the consuming functions, aka filter them beforehand.

[tomazpu]

I was wondering the same. Would it be possible to add to the Manifest struct something like a getter method where only active Environments are returned?

[arthurpitman]

This is a fair point - let's do this!

I just tried with an iterator, but it doesn't really solve the problem: defining it on the Manifest would require changing many function signatures (including this one), definition it on manifest.Environments doesn't improve the problem, as you could still forget to use it. So I think I'll go for an actual getter on the Manifest that does the filtering when called.

[arthurpitman]

I didn't like how this looked, so have tried something different: #1897

[arthurpitman]

Based on feedback, I've gone ahead and tried this in a different way: #1897