Support flat and extended manifest files #130

mtojek · 2020-10-07T12:14:47Z

Issue: elastic/package-spec#55

Changes introduced in the PR:

use ucfg to support both flat and extended manifest files
fix visibility of manifest related structures (all visible)
replace VarValue (scalar/list) with interface{}
fix comments in multiple places (mainly: dataStream)

elasticmachine · 2020-10-07T12:27:12Z

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS

Expand to view the summary

Build stats

Build Cause: [Pull request #130 updated]
Start Time: 2020-10-13T10:51:02.284+0000
Duration: 11 min 50 sec

ycombinator · 2020-10-08T19:22:07Z

internal/packages/packages.go

-// manifest file.
-type VarValue struct {
-	scalar string
-	list   []string


I'm curious why you removed VarValue and replaced it with the broader interface{} type. AFAICT the only types of values allowed for input configuration variables are scalars (int, string, boolean, etc.) or lists of scalars. We don't support maps/dicts. That was the intent behind creating a stricter VarValue type that codified these rules.

It's because of ucfg which (I suppose) doesn't support UnmarshalYAML and MarshalJSON methods. It seemed to me that the codebase would be smaller, although I'm aware of the risk you brought here.

Could you keep VarValue and make it implement https://godoc.org/github.com/elastic/go-ucfg#ConfigUnpacker somehow?

I can try to do this, although I'm not convinced it will work (didn't see this earlier).

I see the benefits of having stricter VarValue type, so if you prefer I can switch back from ucfg to our implementation and extend it with same trick for expanding config map keys as in package-spec.

Thanks, I appreciate you giving it a try. There are a few examples of implementing ConfigUnpacker in the Beats repo. You can find them in your IDE with this regex: func \(.* Unpack\(.*Config.

If you succeed, it would allow us to preserve the strictness of what a variable value can be, which is very nice.

If it doesn't work, let's stick with ucfg and reduce the strictness of what a variable value can be by replacing VarValue with interface{}. I think that's the better tradeoff than implementing some trick to parse flat values with the yaml package.

I think I got it fixed. Let's see what CI will say.

ycombinator · 2020-10-08T19:36:37Z

internal/packages/packages.go

+type Variable struct {
+	Name    string      `config:"name" json:"name" yaml:"name"`
+	Type    string      `config:"type" json:"type" yaml:"type"`
+	Default interface{} `config:"default" json:"default" yaml:"default"`


Now that we're introducing the config struct tags for ucfg's benefit, do we still need to keep the yaml struct tags?

I think you're right, although I have to check whether we don't call yaml.Marshal somewhere...

ycombinator · 2020-10-08T19:37:06Z

internal/packages/packages.go

-type policyTemplate struct {
-	Inputs []input `json:"inputs"`
+// PolicyTemplate is a configuration of inputs responsible for collecting log or metric data.
+type PolicyTemplate struct {


Is the visibility change needed for ucfg to work correctly?

It's due to consistency. If somebody wants to use Manifest structure and extract policy templates to a separate variable, it wouldn't be possible (can't use a package visible field). In general exposed fields should use public structures.

mtojek · 2020-10-12T16:41:14Z

/test

ycombinator

LGTM. WFG.

mtojek · 2020-10-12T17:07:14Z

Something didn't work here:

[2020-10-12T17:03:59.994Z] 2020/10/12 17:03:59 DEBUG found 0 hits in logs-apache.access-ep data stream
[2020-10-12T17:04:00.940Z] 2020/10/12 17:04:00 DEBUG found 0 hits in logs-apache.access-ep data stream
[2020-10-12T17:04:01.885Z] 2020/10/12 17:04:01 DEBUG found 0 hits in logs-apache.access-ep data stream
[2020-10-12T17:04:02.829Z] 2020/10/12 17:04:02 DEBUG found 0 hits in logs-apache.access-ep data stream
[2020-10-12T17:04:03.777Z] System test for apache/access data stream failed

I will investigate it tomorrow.

mtojek · 2020-10-13T09:49:11Z

Ok, I approached the next issue:

[2020-10-13T09:42:07.328Z] Error: error running package system tests: locating data stream root failed: verifying manifest file failed (path: /var/lib/jenkins/workspace/Beats_elastic-package_PR-130/src/github.com/elastic/elastic-package/test/packages/apache/data_stream/status/manifest.yml): reading package manifest failed (path: /var/lib/jenkins/workspace/Beats_elastic-package_PR-130/src/github.com/elastic/elastic-package/test/packages/apache/data_stream/status/manifest.yml): unpacking data stream manifest failed (path: /var/lib/jenkins/workspace/Beats_elastic-package_PR-130/src/github.com/elastic/elastic-package/test/packages/apache/data_stream/status/manifest.yml): type mismatch accessing 'streams.0.vars.0.default' (source:'/var/lib/jenkins/workspace/Beats_elastic-package_PR-130/src/github.com/elastic/elastic-package/test/packages/apache/data_stream/status/manifest.yml')

mtojek · 2020-10-13T11:06:25Z

Ok, it looks that I fixed it finally. VarVariable can be either interface{} or []interface{}. Initially we forgot that's not just string (array of strings).

mtojek added 3 commits October 7, 2020 11:48

Refactor: type visibility, comments

e12c02c

Adjust structures

aea5a49

Use ucfg to support flat and extended manifest files

747015e

mtojek requested a review from ycombinator October 7, 2020 12:14

mtojek self-assigned this Oct 7, 2020

mtojek added 2 commits October 7, 2020 14:36

Fix: make check

47baaa9

Fix: make gomod

b114018

mtojek marked this pull request as ready for review October 7, 2020 13:03

ycombinator reviewed Oct 8, 2020

View reviewed changes

mtojek added 3 commits October 12, 2020 16:37

Merge branch 'master' into 55-support-flat-expanded-objects

ac67717

Bring back VarValue

673cac7

Fix: use ucfg in system test runner

33000f3

ycombinator approved these changes Oct 12, 2020

View reviewed changes

Merge branch 'master' into 55-support-flat-expanded-objects

7ef3755

mtojek added 2 commits October 13, 2020 10:47

Merge branch 'master' into 55-support-flat-expanded-objects

232d268

Fix: use config instead of yaml

bbd33ef

Try: implement Unpacker not ConfigUnpacker

04a04c5

mtojek merged commit 392c5dc into elastic:master Oct 13, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support flat and extended manifest files #130

Support flat and extended manifest files #130

mtojek commented Oct 7, 2020 •

edited

Loading

elasticmachine commented Oct 7, 2020 •

edited

Loading

Build stats

ycombinator Oct 8, 2020 •

edited

Loading

mtojek Oct 8, 2020

ycombinator Oct 8, 2020

mtojek Oct 8, 2020

ycombinator Oct 8, 2020 •

edited

Loading

mtojek Oct 12, 2020

ycombinator Oct 8, 2020

mtojek Oct 8, 2020

ycombinator Oct 8, 2020

mtojek Oct 8, 2020

mtojek commented Oct 12, 2020

ycombinator left a comment

mtojek commented Oct 12, 2020

mtojek commented Oct 13, 2020

mtojek commented Oct 13, 2020

Support flat and extended manifest files #130

Support flat and extended manifest files #130

Conversation

mtojek commented Oct 7, 2020 • edited Loading

elasticmachine commented Oct 7, 2020 • edited Loading

💚 Build Succeeded

Build stats

ycombinator Oct 8, 2020 • edited Loading

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

ycombinator Oct 8, 2020 • edited Loading

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

mtojek commented Oct 12, 2020

ycombinator left a comment

Choose a reason for hiding this comment

mtojek commented Oct 12, 2020

mtojek commented Oct 13, 2020

mtojek commented Oct 13, 2020

mtojek commented Oct 7, 2020 •

edited

Loading

elasticmachine commented Oct 7, 2020 •

edited

Loading

ycombinator Oct 8, 2020 •

edited

Loading

ycombinator Oct 8, 2020 •

edited

Loading