Skip to content
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

sync config via mage #2320

Merged
merged 2 commits into from
Jun 28, 2019
Merged

sync config via mage #2320

merged 2 commits into from
Jun 28, 2019

Conversation

jalvz
Copy link
Contributor

@jalvz jalvz commented Jun 25, 2019

expected by elastic/beats#12618

  • needs beats update
  • needs manual package test

@jalvz jalvz self-assigned this Jun 25, 2019
@graphaelli
Copy link
Member

@jalvz Were you able to build docker container images with this? Can you confirm the slight tweaks we make to the defaults are in the configuration are bundled:

 apm-server:
-  host: "localhost:8200"
+  host: "0.0.0.0:8200"

-  hosts: ["localhost:9200"]
+  hosts: ["elasticsearch:9200"]

@jalvz
Copy link
Contributor Author

jalvz commented Jun 25, 2019

they are not as part of mage config; I thought we have a separate make target doing that?

@graphaelli
Copy link
Member

Generating the config yes, but what is actually included in the container image? I can try it myself but if you already confirmed I can skip it.

@jalvz
Copy link
Contributor Author

jalvz commented Jun 25, 2019

No, I didn't test package yet (only that mage package runs and exits with 0 code) because I expected that if config was right, packages would be as well... Is not the packaging using our apm-server.docker.yml?
If that is not the case, then the image config is surely wrong.

@jalvz
Copy link
Contributor Author

jalvz commented Jun 27, 2019

docker run -it docker.elastic.co/apm/apm-server-oss:8.0.0 grep "localhost:.200" apm-server.yml

host: "localhost:8200"
        # In case you specify and additional path, the scheme is required: http://localhost:9200/path
        # hosts: ["localhost:9200"]
  # In case you specify and additional path, the scheme is required: http://localhost:9200/path
  hosts: ["localhost:9200"]
  # In case you specify and additional path, the scheme is required: http://localhost:9200/path
  #hosts: ["localhost:9200"]

@graphaelli i'd like to move forward with this to unblock things and create an issue for that, if you don't mind

@graphaelli
Copy link
Member

I don't think this should be merged in as is since it breaks the default configuration bundled with the docker image - what else is in this beats update that we need right now?

Also, SNAPSHOT=true PLATFORMS=linux/amd64 mage -v package on my machine is only making it to:

exec: make update

and apparently getting stuck. Removing the PLATFORMS definition allows it to proceed but I haven't tracked down why yet.

@graphaelli
Copy link
Member

graphaelli commented Jun 27, 2019

It seems we can tell mage not to generate the docker configuration, this patch seems to do what we want:

diff --git a/apm-server.docker.yml b/apm-server.docker.yml
index 2e39ad59..627bb108 100644
--- a/apm-server.docker.yml
+++ b/apm-server.docker.yml
@@ -4,7 +4,7 @@
 
 apm-server:
   # Defines the host and port the server is listening on.  use "unix:/path/to.sock" to listen on a unix domain socket.
-  host: "localhost:8200"
+  host: "0.0.0.0:8200"
 
   # Maximum permitted size in bytes of a request's header accepted by the server to be processed.
   #max_header_size: 1048576
@@ -315,7 +315,7 @@ output.elasticsearch:
   # Scheme and port can be left out and will be set to the default (http and 9200)
   # In case you specify and additional path, the scheme is required: http://localhost:9200/path
   # IPv6 addresses should always be defined as: https://[2001:db8::1]:9200
-  hosts: ["localhost:9200"]
+  hosts: ["elasticsearch:9200"]
 
   # Boolean flag to enable or disable the output module.
   #enabled: true
diff --git a/magefile.go b/magefile.go
index 1a02b5f3..952ef2ee 100644
--- a/magefile.go
+++ b/magefile.go
@@ -85,7 +85,7 @@ func Clean() error {
 }
 
 func Config() error {
-	return mage.Config(mage.ShortConfigType|mage.DockerConfigType, ConfigFileParams(), ".")
+	return mage.Config(mage.ShortConfigType, ConfigFileParams(), ".")
 }
 
 func ConfigFileParams() mage.ConfigFileParams {
@@ -93,9 +93,6 @@ func ConfigFileParams() mage.ConfigFileParams {
 		ShortParts: []string{
 			mage.OSSBeatDir("_meta/beat.yml"),
 		},
-		DockerParts: []string{
-			mage.OSSBeatDir("_meta/beat.yml"),
-		},
 	}
 }

Building packages now to verify.

@graphaelli
Copy link
Member

That worked but I liked your idea of generating apm-server.docker.yml - pushed up fcd484e with an implementation for you to consider.

@@ -315,7 +315,7 @@ output.elasticsearch:
# Scheme and port can be left out and will be set to the default (http and 9200)
# In case you specify and additional path, the scheme is required: http://localhost:9200/path
# IPv6 addresses should always be defined as: https://[2001:db8::1]:9200
hosts: ["localhost:9200"]
hosts: ["{{ .elasticsearch_hostport }}"]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

doing the string join with commas in the template was a pain that we can postpone until we really need this to be a list

@jalvz
Copy link
Contributor Author

jalvz commented Jun 28, 2019

excellent, thanks. i had arrived at #2320 (comment), but that would had defeat the purpose. didn't go as far as thinking of ExtraVars 💯

@jalvz jalvz force-pushed the add-mage-target branch 2 times, most recently from f3dd14c to 6746a9b Compare June 28, 2019 07:14
@jalvz jalvz force-pushed the add-mage-target branch from 6746a9b to b92f890 Compare June 28, 2019 07:20
@jalvz jalvz merged commit 67aa5bc into elastic:master Jun 28, 2019
simitt added a commit that referenced this pull request Jul 2, 2019
…#2364)

* Sync config via mage respecting docker defaults
* Update beats framework
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants