Check CAcert file exists when installing extension #761
Conversation
|
We'll also need to add this behavior to the transmitter. Can you add this in this PR or create a new PR for that? |
The transmitter already checks if the file is readable and accessible, is there anything else to do there? I'm going to replicate the transmitter behavior here. |
|
Ah it's already there! Great! Then you can replicate that behavior yes :) |
luismiramirez
force-pushed
the
ca-file-path-ext-download
branch
from
682b048
to
45b32aa
Compare
| @@ -252,13 +252,28 @@ defmodule Mix.Appsignal.Helper do | |||
| end | |||
|
|
|||
| defp download_options do | |||
There was a problem hiding this comment.
If possible, can you add some tests for this in the mix helpers test file?
There was a problem hiding this comment.
It's a bit tricky to test this file. A solution would be to make download_options a public function. I'm not sure if that's worth it.
There was a problem hiding this comment.
For this file it's fine. It's not that apps other than the integration itself can access it. We've made things public in this file before to test them, unfortunately. It would be easier to test in #762, so that we can specify a custom path: paths that don't exist or are not writable.
luismiramirez
force-pushed
the
ca-file-path-ext-download
branch
2 times, most recently
from
72e6e2e
to
2ffb5b6
Compare
The extension installation will no longer fail if the CA cert file is not accessible. It'll print a warning and use the library defaults.
luismiramirez
force-pushed
the
ca-file-path-ext-download
branch
from
2ffb5b6
to
003a2ed
Compare
| @@ -252,13 +252,28 @@ defmodule Mix.Appsignal.Helper do | |||
| end | |||
|
|
|||
| defp download_options do | |||
There was a problem hiding this comment.
For this file it's fine. It's not that apps other than the integration itself can access it. We've made things public in this file before to test them, unfortunately. It would be easier to test in #762, so that we can specify a custom path: paths that don't exist or are not writable.
| Logger.warn( | ||
| "The cacert file path: #{default_cacert_file_path} is not accessible. " <> | ||
| "Reason: #{inspect(message)}. " <> | ||
| "Using system defaults instead." | ||
| ) |
There was a problem hiding this comment.
This should be logged in the download report, but we don't have a field for "warnings" or "messages". Let's keep it like this for now. Can you create an issue for moving this to the download report file?
Looking at the code it's going to a hassle to get this non-error message all the way up to the top again. This is buried quite deep. It's probably easier to do the check twice, earlier in another location so that it's easier to log to the download report file.
The extension installation will no longer fail if the CA cert file is
not accessible. It'll print a warning and use the library defaults.
Fixes: #756