-
Notifications
You must be signed in to change notification settings - Fork 67
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Encode the Kibana saved objects during packaging #157
Conversation
In the packages, the saved objects are stored with decoded JSON fields. The reason is that this makes versioning of it much easier and the diffs are simpler. But inside Kibana some of the fields are stored as encoded json strings (this might change in the future elastic/kibana#43673). To not require special logic on the Package Manager to encode the strings, this is done directly during packaging. One thing not too nice about this PR is that it includes now a dependency on `common.MapStr` from Beats. Reason is that it makes the code much simpler. Part of elastic#42
93cff70
to
687ce86
Compare
go.mod
Outdated
@@ -4,11 +4,15 @@ go 1.12 | |||
|
|||
require ( | |||
github.com/blang/semver v3.5.1+incompatible | |||
github.com/elastic/beats v7.4.2+incompatible |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm actually thinking the same. Move often used functions/types into separate packages and use aliases to not break too much code. But MapStr is not one of my favorite types :), you really need it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really need it is a bendable term ;-) The GetValue
and PutValue
that traverse the tree for dotted keys just make my code much simpler, otherwise I would have to reimplement that.
@@ -0,0 +1,287 @@ | |||
// Licensed to Elasticsearch B.V. under one or more contributor |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@urso Found an "easy" solution. I copied over the mapstr file from libbeat and remove the methods I don't need. Not nice but works.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe create a tech-debt issue for us to know that might need to clean this up in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I filed elastic/beats#14734
@jfsiii After this changed, I assumed I could just get rid of The error I get is:
And the file in question looks as following:
|
Investigating the above further, I stumbled over the following console output:
This is outputting the savedObject after creating on the Kibana side. I wonder why it marks and object instead of a string for |
@ruflin After our video chat and some look at the code here's what I think it happening. TL;DR; Comments should be updated for The asset from #157 (comment) is valid JSON However, that alone isn't enough to submit to the Saved Object HTTP API. I assume you added the required EPM is using the Saved Objects JS API which takes a JS object (as opposed to the JSON string of the asset). It calls JSON.parse to convert the stored JSON string into a JS object that the SO API can use. In doing the EPM adds the required JS SO API requires JS objects. JS objects require calling I don't think we can avoid converting from a string to a JS object since we want to use the SO JS API. We could use the SO HTTP API directly, but that makes EPM much more brittle (duplicating the route locations, losing any validation, etc). We might be able to do something to make I think we should remove the outdated/misleading comments and replace them with something more accurate. Initially, this function was working around bad input (invalid JSON due to golang literals in templates, etc) but now it's for the reasons above. |
@jfsiii Thanks for all the investigation and details. I found the actual error and it was on the registry side. The files were properly encoded but the .tar.gz was created before the encoding happened, so the fix is here: https://github.com/elastic/package-registry/pull/157/files#diff-9421fdf5cec2a108353534181652b35fR167 I tested this with the current Kibana implementation and the cleaned up one. The great thing is both versions of the format work with the current implementation, so if we merge this PR, things should keep working. |
dev/generator/main_test.go
Outdated
// DecodeExported decodes an exported dashboard | ||
//func EncodeKibanaAssets(result MapStr) MapStr { | ||
|
||
func TestFoo(t *testing.T) { // Read file from json |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I'm reading this correctly, the test ensures no errors occur when a) reading the test file b) calling encodedSavedObject
.
If it doesn't already, it would be useful to test if the output of the function is what's expected; not only that it doesn't fail.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree. My plan is to have a more static test file and check the generated output to compare it. At the moment I link to an existing file which will probably not be there anymore. Will merge as is and follow up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left two questions, but happy to see this merge.
With elastic/package-registry#157 issue elastic/package-registry#42 has been resolved. This means the code to encode some of the Kibana fields is not needed anymore.
With elastic/package-registry#157 issue elastic/package-registry#42 has been resolved. This means the code to encode some of the Kibana fields is not needed anymore.
In the packages, the saved objects are stored with decoded JSON fields. The reason is that this makes versioning of it much easier and the diffs are simpler. But inside Kibana some of the fields are stored as encoded json strings (this might change in the future elastic/kibana#43673). To not require special logic on the Package Manager to encode the strings, this is done directly during packaging.
One thing not too nice about this PR is that it includes now a dependency on
common.MapStr
from Beats. Reason is that it makes the code much simpler.Part of #42