Rework internal config structure #261

patrickpichler · 2021-03-19T14:19:25Z

This PR contains a proof of concept of a new configuration scheme.

It basically breaks with the current one completely. For further information have a look at
discussion #165

I opened this PR just to make my code changes more visible.

patrickpichler · 2021-03-26T14:36:10Z

This PR is getting more and more in a state where we could merge it.
There is now a conversion layer between the old config structs and the new ones. I
hooked up the CLI to convert the converter and use the new deployment logic.

There is also now the possibility to test the new config syntax, by setting the NEW_CONFIG
env variable to 1.

What is still missing for now are tests. I'm now focusing on those.

@dynatrace-oss/mac-maintainers would be nice if somebody could read through the PR

github-actions · 2021-04-06T10:32:20Z

❌ Integration tests failed on ubuntu-latest

github-actions · 2021-04-06T10:45:01Z

❌ Integration tests failed on ubuntu-latest

github-actions · 2021-04-09T07:01:50Z

🎉 Integration tests ran successfully on ubuntu-latest 🥳

github-actions · 2021-04-09T11:22:33Z

🎉 Integration tests ran successfully on ubuntu-latest 🥳

UnseenWizzard · 2021-04-14T18:35:03Z

pkg/version/version.go

@@ -16,4 +16,4 @@

 package version

-const MonitoringAsCode = "1.5.0"
+const MonitoringAsCode = "config-poc"


random note cause I started by scrolling to the bottom: this should change before merging this ;)

tatomir146 · 2021-07-13T06:49:25Z

pkg/config/v2/config.go

+const (
+	// id is a special parameter. it is not allowed to be set via the config,
+	// but needs to work as normal parameter otherwise (e.g. it can be referenced).
+	ID_PARAMETER = "id"


Goland complains we shoud use camel and not snake case for these constants.

TIL that golang uses camel case as naming convention for constants.
THX! I will fix it!

tatomir146 · 2021-07-13T06:50:01Z

pkg/config/v2/config.go

+)
+
+const (
+	// id is a special parameter. it is not allowed to be set via the config,


These comments should start with
// ID_PARAMETER is a special...

tatomir146 · 2021-07-13T06:51:16Z

pkg/config/v2/config.go

+	return c.Coordinate.Match(ref)
+}
+
+// test if this config has a dependency on the given config.


Same for method comments, they should start with method name
// HasDependencyOn

tatomir146 · 2021-07-13T06:51:58Z

pkg/config/v2/config.go

+	// map of all parameters which will be resolved and are then available
+	// in the template
+	Parameters Parameters
+	// slice of references as specified by the paramters. needed so that the


Suggested change

// slice of references as specified by the paramters. needed so that the

// slice of references as specified by the parameters. needed so that the

tatomir146 · 2021-07-13T06:53:44Z

pkg/config/v2/config_loader.go

+}
+
+func parseSkip(context *ConfigLoaderContext, environment manifest.EnvironmentDefinition,
+	configId string, definition configDefinition, param interface{}) (bool, error) {


Unused param

Suggested change

configId string, definition configDefinition, param interface{}) (bool, error) {

configId string, param interface{}) (bool, error) {

tatomir146 · 2021-07-13T06:54:14Z

pkg/config/v2/config_loader.go

+}
+
+func parseParameter(context *ConfigLoaderContext, environment manifest.EnvironmentDefinition,
+	configId string, definition configDefinition, name string, param interface{}) (parameter.Parameter, error) {


Unused param

Suggested change

configId string, definition configDefinition, name string, param interface{}) (parameter.Parameter, error) {

configId string, name string, param interface{}) (parameter.Parameter, error) {

tatomir146 · 2021-07-13T06:55:16Z

pkg/config/v2/config_writer.go

+		}
+
+		switch valueParam.Value.(type) {
+		// map/array values need special handling to not collide with oter paramters


other parameters

tatomir146 · 2021-07-13T06:57:46Z

pkg/config/v2/coordinate/coordinate.go

+}
+
+// tests if this coordinate is the same as the given one
+func (c *Coordinate) Match(coordinate Coordinate) bool {


Using different receiver names

tatomir146 · 2021-07-13T07:25:02Z

pkg/deploy/v2/deploy.go

+
+type knownEntityMap map[string]map[string]struct{}
+
+// deployes the given configs with the given apis via the given client


Suggested change

// deployes the given configs with the given apis via the given client

// DeployConfigs deploys the given configs with the given apis via the given client

tatomir146 · 2021-07-13T07:25:52Z

pkg/deploy/v2/deploy.go

+		// as they will never be resolved before we validate
+		// the parameters
+		if ref.Config == configCoordinates {
+			// parameters referencing themselfs makes no sense


Suggested change

// parameters referencing themselfs makes no sense

// parameters referencing themselves makes no sense

tatomir146 · 2021-07-13T07:27:34Z

pkg/environment/environment_loader.go

@@ -33,7 +33,7 @@ func LoadEnvironmentList(specificEnvironment string, environmentsFile string, fs

 	environmentsFromFile, errorList := readEnvironments(environmentsFile, fs)

-	if environmentsFromFile == nil || len(environmentsFromFile) == 0 {
+	if len(environmentsFromFile) == 0 {


Can we fix the typo on line 30:
"no environment file provided"

tatomir146 · 2021-07-13T07:28:02Z

pkg/manifest/manifest.go

+}
+
+func (m *Manifest) GetEnvironmentsAsSlice() []EnvironmentDefinition {
+	var result []EnvironmentDefinition = make([]EnvironmentDefinition, 0, len(m.Environments))


Suggested change

var result []EnvironmentDefinition = make([]EnvironmentDefinition, 0, len(m.Environments))

var result = make([]EnvironmentDefinition, 0, len(m.Environments))

tatomir146 · 2021-07-13T07:33:49Z

pkg/util/client/dummy_client.go

+	"path/filepath"
+	"time"
+
+	api "github.com/dynatrace-oss/dynatrace-monitoring-as-code/pkg/api"


Suggested change

api "github.com/dynatrace-oss/dynatrace-monitoring-as-code/pkg/api"

"github.com/dynatrace-oss/dynatrace-monitoring-as-code/pkg/api"

tatomir146 · 2021-07-14T08:50:01Z

pkg/config/config.go

@@ -88,6 +87,11 @@ func NewConfigForDelete(id string, fileName string, properties map[string]map[st
 	return newConfig(id, "", nil, filterProperties(id, properties), api, fileName)


NewConfigForDelete seems to not be used anywhere.

This is legacy config file. I would rather not touch it any more than necessary 😆
I guess the plan would be, to get rid of all legacy files within the next few versions.

tatomir146 · 2021-07-15T05:54:39Z

pkg/version/version.go

@@ -16,4 +16,4 @@

 package version

-const MonitoringAsCode = "1.6.0"
+const MonitoringAsCode = "2.0.0-new-config"


why not simply

Suggested change

const MonitoringAsCode = "2.0.0-new-config"

const MonitoringAsCode = "2.0.0"

This was just for local debug builds. I would not go with 2.0.0 yet.
Maybe 2.0.0-next, since we are going to build next binaries. That
way people don't mistake it for a full release.

tatomir146 · 2021-07-15T05:57:20Z

cmd/monaco/main.go

+				Aliases: []string{"v"},
+			},
+			&cli.StringFlag{
+				Name:    "specific-environment",


This would be a good opportunity to modify this: how about using -e or -environment. Not sure we need specific

If you ask me, we should delete this file anyway. The new main is located
in cmd/monaco/v2/main.go.

I just kept this file as I wasn't sure how we want to go on with it.

tatomir146 · 2021-07-15T06:19:42Z

cmd/monaco/main.go

-	deployCommand := getDeployCommand(fs)
+	var deployCommand cli.Command
+
+	if isEnvFlagEnabled("NEW_CONFIG") {


Shouldn't we make NEW_CONFIG a default? We should rather allow compatibility mode with CONFIG_V1 flag, but use new config as default.

As pointed out above, I would ignore this file and delete it.

goberchtold · 2021-07-15T06:37:07Z

cmd/monaco/v2/deploy/cmd.go

+	})
+
+	if errs != nil {
+		return errors.New("error while loading projects")


Suggested change

return errors.New("error while loading projects")

util.PrintErrors(errs)

return errors.New("error while loading projects")

Is there any reason not to print the errors? Just dropping them and returning a new error doesn't seem very favorable

Good catch 😆

goberchtold · 2021-07-15T06:37:53Z

cmd/monaco/v2/deploy/cmd.go

+	sortedConfigs, errs := sort.GetSortedConfigsForEnvironments(projects, environmentNames)
+
+	if errs != nil {
+		return errors.New("error during sort")


Suggested change

return errors.New("error during sort")

util.PrintErrors(errs)

return errors.New("error during sort")

goberchtold · 2021-07-15T06:44:52Z

cmd/monaco/v2/deploy/cmd.go

+		for _, err := range errs {
+			util.Log.Error(err.Error())
+		}


Suggested change

for _, err := range errs {

util.Log.Error(err.Error())

}

util.PrintErrors(errs)

goberchtold · 2021-07-15T06:46:56Z

cmd/monaco/v2/deploy/cmd.go

+		if errors != nil {
+			deploymentErrors = append(deploymentErrors, errors...)
+		}


Suggested change

if errors != nil {

deploymentErrors = append(deploymentErrors, errors...)

}

deploymentErrors = append(deploymentErrors, errors...)

No need to check for nil, appending a nil slice is perfectly fine

goberchtold · 2021-07-15T06:53:49Z

cmd/monaco/v2/deploy/cmd.go

+		for _, err := range generalErrors {
+			util.Log.Error(util.ErrorString(err))
+		}


Suggested change

for _, err := range generalErrors {

util.Log.Error(util.ErrorString(err))

}

util.PrintErrors(generalErrors)

This needs to change anyway. We should create a proper error report here.
I'll add an TODO.

goberchtold · 2021-07-15T08:42:31Z

pkg/manifest/manifest_loader.go

+	return result, nil
+}
+
+func parseProjectDefinition(context *projectLoaderContext, project project) ([]ProjectDefinition, []error) {


As I see it only one error is ever returned, is a slice really needed here?

As this method is designed with adding new project types in mind,
I would personally keep it.

goberchtold · 2021-07-15T08:44:06Z

pkg/project/v2/sort/sort.go

+	if errs != nil {
+		errors = append(errors, errs...)
+	}


Suggested change

if errs != nil {

errors = append(errors, errs...)

}

errors = append(errors, errs...)

We should short-circuit anyway here, as the sortedProjectsPerEnvironment will anyway be nil.

goberchtold · 2021-07-15T08:44:58Z

pkg/writer/writer.go

+		if len(errs) > 0 {
+			errors = append(errors, errs...)
+		}


Suggested change

if len(errs) > 0 {

errors = append(errors, errs...)

}

errors = append(errors, errs...)

goberchtold · 2021-07-15T08:45:33Z

pkg/writer/writer.go

+func collectAllConfigs(p project.Project) []config.Config {
+	var result []config.Config


Suggested change

func collectAllConfigs(p project.Project) []config.Config {

var result []config.Config

func collectAllConfigs(p project.Project) (result []config.Config) {

goberchtold · 2021-07-15T08:48:03Z

pkg/util/client/dummy_client.go

+
+func (c *DummyClient) BulkDeleteByName(a api.Api, names []string) error {
+	for _, name := range names {
+		err := c.DeleteByName(a, name)


The dummy client's DeleteByName only returns nil error, so no need to check for that tbh

Yeah, for now. The interface specifies that it could return an error,
so imho we should handle it (even though it is pretty much useless
now ^^)

As the current config structure was a bit complicated to use, it has been completely reworked. First of all, there is now a central manifest file, which specifies all the details needed for deployment. This includes a list of all available projects (including paths to them, so goodbye recursive project discovery) and a grouping of available environment (including the name of the env variable holding the token). The structure within a project now has to follow a strict scheme, naminly `[api]/[config]`. Auto-discovery of sub-Projects are no longer supported. This was done to make things more explicit. The configuration yaml also looks now different. Instead of splitting configuration definitions in two parts (template location, rest of the config), everything is now in one structure. The top level structure of each yaml file is now called `configs`. For examples please see the documentation. Config paramters also have been reworked. They now support more values besides strings. There are some special parameter though. One of this special parameter is the reference parameter. References no longer depend on a special formatted string, instead you have to explicitly define the location of the config you want to reference. It is now also possible to depend on **any** reference of a config, not just id and name. Another special parameter is the environment parameter. It is **no longer** possible to reference env variables via the `{ .env.XYZ }` syntax in the config yaml or the template. This decision was made, to make configs more explicit. For more details on how the new parameter syntax works, please see the documentation. The delete.yaml now also supports deleting configs with multiple `/` in its name. Fixes Dynatrace#351 Fixes Dynatrace#255

Since we reworked the configuration syntax, the current configuration file syntax won't work. In order to support different types of integration tests, they have been moved into their own package. Also all the command parsing logic of the main file was factored out into a runner package. This way we can easily use it from the integration test packages.

patrickpichler force-pushed the poc-new-config branch 3 times, most recently from 7fe58d3 to a4cf2cf Compare March 26, 2021 14:31

patrickpichler force-pushed the poc-new-config branch from 3bb1c97 to a20e811 Compare March 30, 2021 05:46

patrickpichler requested a review from a team March 30, 2021 05:46

patrickpichler force-pushed the poc-new-config branch 4 times, most recently from f1af432 to 8d9f817 Compare April 6, 2021 10:27

patrickpichler added the run-e2e-test Manually trigger the E2E tests for reviewed PRs label Apr 6, 2021

patrickpichler added run-e2e-test Manually trigger the E2E tests for reviewed PRs and removed run-e2e-test Manually trigger the E2E tests for reviewed PRs labels Apr 6, 2021

patrickpichler force-pushed the poc-new-config branch from 698d162 to 1b9d73c Compare April 9, 2021 06:52

patrickpichler marked this pull request as ready for review April 9, 2021 06:54

patrickpichler added run-e2e-test Manually trigger the E2E tests for reviewed PRs and removed run-e2e-test Manually trigger the E2E tests for reviewed PRs labels Apr 9, 2021

patrickpichler force-pushed the poc-new-config branch 2 times, most recently from 49e73b3 to 02b591d Compare April 9, 2021 11:18

patrickpichler added run-e2e-test Manually trigger the E2E tests for reviewed PRs and removed run-e2e-test Manually trigger the E2E tests for reviewed PRs labels Apr 9, 2021

patrickpichler force-pushed the poc-new-config branch from 68a7196 to 7bafac0 Compare April 9, 2021 11:26

patrickpichler changed the title ~~Proof of concept for new config format~~ Rework internal config structure Apr 13, 2021

UnseenWizzard reviewed Apr 14, 2021

View reviewed changes

patrickpichler requested a review from a team April 14, 2021 18:49

patrickpichler force-pushed the poc-new-config branch from 7bafac0 to 1c28d82 Compare April 16, 2021 05:26

patrickpichler force-pushed the poc-new-config branch from 54c5194 to 1572399 Compare July 9, 2021 08:07

tatomir146 self-requested a review July 13, 2021 06:30

tatomir146 previously approved these changes Jul 13, 2021

View reviewed changes

tatomir146 reviewed Jul 13, 2021

View reviewed changes

patrickpichler dismissed tatomir146’s stale review via e064d77 July 13, 2021 08:00

patrickpichler force-pushed the poc-new-config branch from 1572399 to e064d77 Compare July 13, 2021 08:00

tatomir146 reviewed Jul 14, 2021

View reviewed changes

tatomir146 reviewed Jul 15, 2021

View reviewed changes

patrickpichler force-pushed the poc-new-config branch 2 times, most recently from 1b119ef to fc0a49c Compare July 15, 2021 07:06

goberchtold reviewed Jul 15, 2021

View reviewed changes

patrickpichler force-pushed the poc-new-config branch 2 times, most recently from e941c0a to ad0117a Compare July 20, 2021 13:51

tatomir146 self-requested a review July 20, 2021 13:54

tatomir146 previously approved these changes Jul 20, 2021

View reviewed changes

patrickpichler dismissed tatomir146’s stale review via bae85be July 20, 2021 14:17

patrickpichler force-pushed the poc-new-config branch from ad0117a to bae85be Compare July 20, 2021 14:17

patrickpichler removed the run-e2e-test Manually trigger the E2E tests for reviewed PRs label Jul 20, 2021

patrickpichler force-pushed the poc-new-config branch from bae85be to f772b33 Compare July 20, 2021 14:18

Patrick Pichler added 2 commits July 20, 2021 16:30

patrickpichler force-pushed the poc-new-config branch from f772b33 to f4ceb79 Compare July 20, 2021 14:31

tatomir146 approved these changes Jul 21, 2021

View reviewed changes

patrickpichler merged commit fcfdf99 into Dynatrace:next Jul 21, 2021

patrickpichler deleted the poc-new-config branch July 21, 2021 05:17

	// slice of references as specified by the paramters. needed so that the
	// slice of references as specified by the parameters. needed so that the

	configId string, definition configDefinition, param interface{}) (bool, error) {
	configId string, param interface{}) (bool, error) {

	configId string, definition configDefinition, name string, param interface{}) (parameter.Parameter, error) {
	configId string, name string, param interface{}) (parameter.Parameter, error) {


		type knownEntityMap map[string]map[string]struct{}

		// deployes the given configs with the given apis via the given client

	// deployes the given configs with the given apis via the given client
	// DeployConfigs deploys the given configs with the given apis via the given client

	// parameters referencing themselfs makes no sense
	// parameters referencing themselves makes no sense

	var result []EnvironmentDefinition = make([]EnvironmentDefinition, 0, len(m.Environments))
	var result = make([]EnvironmentDefinition, 0, len(m.Environments))

	api "github.com/dynatrace-oss/dynatrace-monitoring-as-code/pkg/api"
	"github.com/dynatrace-oss/dynatrace-monitoring-as-code/pkg/api"

		@@ -88,6 +87,11 @@ func NewConfigForDelete(id string, fileName string, properties map[string]map[st
		return newConfig(id, "", nil, filterProperties(id, properties), api, fileName)

	const MonitoringAsCode = "2.0.0-new-config"
	const MonitoringAsCode = "2.0.0"

	return errors.New("error while loading projects")
	util.PrintErrors(errs)
	return errors.New("error while loading projects")

	return errors.New("error during sort")
	util.PrintErrors(errs)
	return errors.New("error during sort")

		func collectAllConfigs(p project.Project) []config.Config {
		var result []config.Config

Rework internal config structure #261

Rework internal config structure #261

Conversation

patrickpichler commented Mar 19, 2021 • edited Loading

patrickpichler commented Mar 26, 2021

github-actions bot commented Apr 6, 2021

❌ Integration tests failed on ubuntu-latest

github-actions bot commented Apr 6, 2021

❌ Integration tests failed on ubuntu-latest

github-actions bot commented Apr 9, 2021

🎉 Integration tests ran successfully on ubuntu-latest 🥳

github-actions bot commented Apr 9, 2021

🎉 Integration tests ran successfully on ubuntu-latest 🥳

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patrickpichler commented Mar 19, 2021 •

edited

Loading