feat: docker parameters for seeds #317

noaccOS · 2023-07-05T16:29:16Z

add environment variables to docker compose to be able to run the seeds with working astarte parameters

coveralls · 2023-07-05T16:32:15Z

Pull Request Test Coverage Report for Build 1b397325d64f5a1e95d8d8a7fef80c144069d8bb-PR-317

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

For more information on this, see Tracking coverage changes with pull request builds.
To avoid this issue with future PRs, see these Recommended CI Configurations.
For a quick fix, rebase this PR at GitHub. Your next report should be accurate.

Details

0 of 0 changed or added relevant lines in 0 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage remained the same at 83.901%

Totals
Change from base Build af596578246434293a5ee65433169320715a5c96:	0.0%
Covered Lines:	1428
Relevant Lines:	1702

💛 - Coveralls

rbino · 2023-07-11T15:50:27Z

@noaccOS I'd keep an opt-in approach here: your PR is useful if I actually want to use seeds to connect to a real Astarte instance, but I would keep the usecase of just wanting some random seeds to be able to bring up Edgehog.
So you should fall back to default hardcoded values instead of failing if the env variables are not present

rbino · 2023-08-07T15:22:37Z

backend/priv/keys/tenant_private.pem

+-----BEGIN EC PRIVATE KEY-----
+MHcCAQEEICx5W2odFd5CyMTv5VlLW96fgvWtcJ3bIJVVc3GWhMHBoAoGCCqGSM49
+AwEHoUQDQgAEhV0KI4hByk0uDkCg4yZImMTiAtz2azmpbh0sLAKOESdlRYOFw90U
+p4F9fRRV5Li6Pn5XZiMCZhVkS/PoUbIKpA==
+-----END EC PRIVATE KEY-----


Put this in a seeds subdirectory (i.e. priv/repo/seeds/realm_private.pem) so it's clear it's only for development purposes

rbino · 2023-08-07T15:25:03Z

backend/priv/keys/realm_private.pem

@@ -0,0 +1 @@
+notaprivatekey


See other comment for the subdirectory

rbino · 2023-08-07T15:28:46Z

docker-compose.yml

+      IPBASE_API_KEY: "${IPBASE_API_KEY}"
+      GOOGLE_GEOLOCATION_API_KEY: "${GOOGLE_GEOLOCATION_API_KEY}"
+      GOOGLE_GEOCODING_API_KEY: "${GOOGLE_GEOCODING_API_KEY}"
+      S3_ACCESS_KEY_ID: "${S3_ACCESS_KEY_ID}"
+      S3_SECRET_ACCESS_KEY: "${S3_SECRET_ACCESS_KEY}"
+      S3_REGION: "${S3_REGION}"
+      S3_SCHEME: "${S3_SCHEME}"
+      S3_HOST: "${S3_HOST}"
+      S3_PORT: "${S3_PORT}"
+      S3_BUCKET: "${S3_BUCKET}"
+      S3_ASSET_HOST: "${S3_ASSET_HOST}"
+      S3_GCP_CREDENTIALS: "${S3_GCP_CREDENTIALS}"
+      SEEDS_REALM: "${SEEDS_REALM}"
+      SEEDS_REALM_PRIVATE_KEY_FILE: /keys/realm_private.pem
+      SEEDS_TENANT_PRIVATE_KEY_FILE: /keys/tenant_private.pem
+      SEEDS_ASTARTE_BASE_API_URL: "${SEEDS_ASTARTE_BASE_API_URL}"


Since you already have an .env file, I think you can just use

env_file: ./filename.env

In that case, make the filename not hidden to make it more evident (e.g. compose.env like we do in Astarte)

Docker by default only reads the .env file for the compose variables, and it is not possible to change that (at least I haven't found a way in the docs).
To be able to use a custom.env file, the user would need to pass the option -f custom.env with each command.

This is in contrast with Astarte's compose file, where it is possible to use another file, because here we're reading the variables in the compose file directly https://github.com/edgehog-device-manager/edgehog/pull/317/files#diff-e45e45baeda1c1e73482975a664062aa56f20c03dd9d64a827aba57775bed0d3R62 instead of just redirecting the variables to the containers.

As for using the env_file option, I'm not against that but I'll need to rename the SEEDS_*_PRIVATE_KEY_FILE as the same name is used in the .env.
Not necessarily a bad thing as it might be ambiguous as it is now.

Ok I get it, in that case let's leave everything as is (no need to split between env_file and .env since it just disperses things)

rbino · 2023-08-07T15:30:13Z

backend/priv/repo/seeds.exs

+        Logger.warning(
+          """
+          The realm's private key is not a valid RSA/RC private key. \
+          This instante will not be able to connect to Astarte.


rbino · 2023-08-07T15:32:07Z

backend/priv/repo/seeds.exs

+          The realm's private key is not a valid RSA/RC private key. \
+          This instante will not be able to connect to Astarte.
+          """
+          |> String.trim_trailing("\n")


If you want to split warning messages on multiple lines you can do

Logger.warning("My very very long" <> "message" )

(not sure about the formatting, but mix format will take care of that). Split it at < 100 columns.

rbino · 2023-08-07T15:34:01Z

backend/priv/repo/seeds.exs

+
+read_file_from_env_var = fn env_var ->
+  System.get_env(env_var, "")
+  |> File.read()


File.read!() already fails with helpful messages and mentioning the filename, just use that

rbino · 2023-08-07T15:34:15Z

backend/priv/repo/seeds.exs

@@ -25,19 +25,40 @@ alias Edgehog.{
  Tenants
 }

+require Logger
+
+read_file_from_env_var = fn env_var ->


Use a trailing ! since this can raise

rbino · 2023-08-07T15:34:53Z

backend/priv/repo/seeds.exs

+require Logger
+
+read_file_from_env_var = fn env_var ->
+  System.get_env(env_var, "")


Since we already raise, use System.fetch_env!

rbino · 2023-08-07T15:35:56Z

backend/priv/repo/seeds.exs

+    """
+    Using default tenant private key. \
+    Please be sure to avoid using this for production.
+    """
+    |> String.trim_trailing("\n")


See other comment on multiline Logger.warning

rbino · 2023-08-07T15:37:32Z

backend/priv/repo/seeds.exs

+  read_file_from_env_var.("SEEDS_REALM_PRIVATE_KEY_FILE")
+  |> then(fn pk ->
+    case X509.PrivateKey.from_pem(pk) do


You can pipe into case, so you drop then:

read_file(...) |> X509.PrivateKey.from_pem() |> case do ...

I cannot do that as I need pk to return it in case of error

edgehog/backend/priv/repo/seeds.exs

Line 90 in 1b39732

pk

Unfortunately piping wouldn't allow me to do it

Keeps defaults in a single point Signed-off-by: Francesco Noacco <francesco.noacco@secomind.com>

Signed-off-by: Francesco Noacco <francesco.noacco@secomind.com>

noaccOS force-pushed the dynamic-seeds branch from eb2c39e to deea557 Compare July 5, 2023 16:41

noaccOS mentioned this pull request Jul 6, 2023

feat: Edgehog in 5 minutes #319

Merged

noaccOS force-pushed the dynamic-seeds branch 2 times, most recently from 16e6a5e to 27f2352 Compare July 10, 2023 07:35

noaccOS force-pushed the dynamic-seeds branch from 25914d3 to 9c52f63 Compare July 12, 2023 14:59

noaccOS force-pushed the dynamic-seeds branch from 9c52f63 to ae8d498 Compare July 20, 2023 08:48

noaccOS force-pushed the dynamic-seeds branch 4 times, most recently from a4d71f2 to 1b39732 Compare July 28, 2023 14:17

rbino requested changes Aug 7, 2023

View reviewed changes

noaccOS force-pushed the dynamic-seeds branch 2 times, most recently from 266a60e to 24f613a Compare August 10, 2023 13:23

noaccOS requested a review from rbino August 10, 2023 13:32

noaccOS force-pushed the dynamic-seeds branch from 24f613a to 471e762 Compare August 10, 2023 13:40

noaccOS added 2 commits August 21, 2023 16:44

refactor(docker-compose): use .env file for env variables

d3d527e

Keeps defaults in a single point Signed-off-by: Francesco Noacco <francesco.noacco@secomind.com>

feat(seeds): use env vars from docker-compose

f984e72

Signed-off-by: Francesco Noacco <francesco.noacco@secomind.com>

noaccOS force-pushed the dynamic-seeds branch from 471e762 to f984e72 Compare August 21, 2023 14:44

rbino approved these changes Oct 5, 2023

View reviewed changes

rbino merged commit ef663af into edgehog-device-manager:main Oct 5, 2023
4 checks passed

noaccOS deleted the dynamic-seeds branch October 22, 2024 12:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: docker parameters for seeds #317

feat: docker parameters for seeds #317

noaccOS commented Jul 5, 2023

coveralls commented Jul 5, 2023 •

edited

Loading

rbino commented Jul 11, 2023

rbino Aug 7, 2023

rbino Aug 7, 2023

rbino Aug 7, 2023

noaccOS Aug 7, 2023

rbino Aug 7, 2023

rbino Aug 7, 2023

rbino Aug 7, 2023

rbino Aug 7, 2023

rbino Aug 7, 2023

rbino Aug 7, 2023

rbino Aug 7, 2023

rbino Aug 7, 2023

noaccOS Aug 9, 2023

		@@ -0,0 +1 @@
		notaprivatekey

feat: docker parameters for seeds #317

feat: docker parameters for seeds #317

Conversation

noaccOS commented Jul 5, 2023

coveralls commented Jul 5, 2023 • edited Loading

Pull Request Test Coverage Report for Build 1b397325d64f5a1e95d8d8a7fef80c144069d8bb-PR-317

Warning: This coverage report may be inaccurate.

Details

💛 - Coveralls

rbino commented Jul 11, 2023

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

coveralls commented Jul 5, 2023 •

edited

Loading