-
Notifications
You must be signed in to change notification settings - Fork 102
Promoting integrations packages to snapshot #133
Promoting integrations packages to snapshot #133
Conversation
Looks like CI failed because the Kibana docker container didn't come up: https://beats-ci.elastic.co/blue/organizations/jenkins/Beats%2Fpackage-storage/detail/PR-133/1/tests |
Hmmm, doesn't look like the CI failure is ephemeral. Investigating... |
I re-ran the test command locally.
I was able to reproduce the failure starting up the Kibana container. Checking the container's logs:
@ph @skh any ideas what's going on here? Maybe something in https://github.com/elastic/package-storage/blob/master/testing/environments/kibana.config.yml needs to be changed? |
Yes.
|
de786d2
to
9e2ba06
Compare
CI failures seem relevant. Investigating... |
I looked into why the tests are failing in CI. First I ran
So next I spun up the same test environment as spun up by the
And then manually tried to execute the line in the test that was failing: package-storage/testing/main_integration_test.go Lines 102 to 113 in 8ac7650
Still investigating what's going on but I don't know these waters too well. @ph @skh if the above rings any bells for you, please let me know! |
@neptunian is that message coming from the refactor of the installation? |
@ph I wonder if it has anything to do with the fact that all integrations packages (except
As a test I've added |
@@ -128,7 +128,7 @@ type Package struct { | |||
} | |||
|
|||
func getPackages(t *testing.T) ([]string, error) { | |||
resp, err := http.Get("http://localhost:8080/search") | |||
resp, err := http.Get("http://localhost:8080/search?experimental=true") |
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.
@ph Does this change make sense to you?
I'm able to reproduce the latest
This is the same error we are getting with the above API call for the following packages:
However, the same API call succeeds for the following packages:
|
This error comes from this method: https://github.com/elastic/kibana/blob/master/src/plugins/discover/server/saved_objects/search_migrations.ts#L24 It happens on searches that contain a I currently happens only on searches, because the piece of code that crashes when it encounters these It only happens now because the migration in question has been added quite recently. The underlying questions are, with the amount of unescaped
|
Thanks so much for digging into this error @skh. To try and narrow down the surface area of what might've changed to cause this error now, I'm going to test these packages with an older Kibana. |
I tried running the same |
One of the packages that's failing the tests is However, looking at the same files in the So it appears that somehow when the packages were copied from the Digging around some more I found this PR: #66. Indeed, one of the changes in there is to "encode dashboards" and you can clearly see the escaping (aka encoding) of So we need to do the same encoding/escaping in this PR here now. |
Thanks to @ph and @jonathan-buttner for pointing me to https://github.com/elastic/integrations/blob/master/magefile.go#L253. I ran that target and copied over the generated files (minus the |
Hmmm, now we're up to 21 CI test failures 😞 . Investigating... |
Regarding the new 500 errors, they are happening because of this underlying error:
@skh pointed me to elastic/kibana#71727, which was merged ~4 hours ago. It is likely the latest I will try again in ~24 hours. |
New build of Kibana is available. Re-running CI job... |
The code to generate example packages was removed some time ago but the docs were forgotten. This cleans it up.
Promotes all available packages from elastic/integrations#176 to snapshot registry.
Script used: