chore: upgrade Elixir to v1.20, erlang to v29 - #795
Conversation
Update elixir from 1.17.3-otp-27 to 1.20.2-otp-29. Update erlang from 27.3.4 to 29.0.2. Version 0.18 of `faker` is incompatible with elixir 1.20, which necessitated an upgrade. Update Logger configuration & install `logger_backends`, which is where the pre-existing logging backend system has moved to. Elixir's built-in `JSON` module that was added in v1.18 replaces most usages of `Jason` in the codebase. `Jason` has been left as a dependency for screens configuration saving (used for emergency takeover setting) - `JSON` does not support a pretty-printing, which is relied on to save a well-formed configuration file. `:json.format` was considered, but does not transform `nil` to `null`, which `Jason.encode!()` does. Given that we are looking to move this configuration to Postgres, keep `Jason` for the time being, until the migration is complete and we may revisit. Reorder lists in tests whose ordering has changed as a result of changes in erlang. Elixir v1.20 was able to detect unused `require Logger` statements, which are removed. Add a Phoenix code reloading listener to suppress warnings that were printed to STDOUT after the upgrade when running the dev server.
| @moduledoc false | ||
|
|
||
| @spec upload_takeover_image(String.t(), binary(), String.t()) :: :ok | ||
| def upload_takeover_image(_alert_id, _image_data, _image_type), do: :ok |
There was a problem hiding this comment.
When running mix test with the original version of this function, we get the following:
warning: the following clause cannot match because the previous clauses already matched all possible values:
error ->
it attempts to match on the result of:
Screenplay.EmergencyTakeoverTool.Images.TestFetch.upload_takeover_image(
alert_id,
image_binary,
image_type
)
which has the already matched type:
dynamic(:ok)
where "error" was given the type:
# type: dynamic(:ok)
# from: lib/screenplay_web/controllers/emergency_takeover_tool/alert_controller.ex:199:17
error
type warning found at:
│
199 │ error -> {:halt, error}
│ ~
│
└─ lib/screenplay_web/controllers/emergency_takeover_tool/alert_controller.ex:199:23: ScreenplayWeb.EmergencyTakeoverTool.AlertController.upload_takeover_images/2
Is there a better way to appease the type system here?
There was a problem hiding this comment.
I'm thinking of this almost as a warning that the calling code is untestable, because the :error branch can never be reached when calling the test version of the function, which always returns :ok. A possible solution would be to make the function return :error when a specific sentinel value is supplied as one of the arguments. (That alone would probably resolve the compiler error without actually forcing us to add test coverage of the branch, but that would be a logical next step.)
|
|
||
| # Configures Elixir's Logger | ||
| config :logger, :console, | ||
| config :logger, :default_handler, |
There was a problem hiding this comment.
Curious about the change from console to default_handler/default_formatter here — this wasn't something I encountered when I upgraded RTS. The actual items being configured (at least the format strings) seem like they'd only be relevant to the console logger. And what is the difference between :default_handler, format: "..." and :default_formatter, format: "..."?
There was a problem hiding this comment.
And what is the difference between :default_handler, format: "..." and :default_formatter, format: "..."?
That's a goof on my part - this should have been default_formatter on this line. That part I've fixed in 5ae5bdb
Curious about the change from console to default_handler/default_formatter here — this wasn't something I encountered when I upgraded RTS. The actual items being configured (at least the format strings) seem like they'd only be relevant to the console logger.
I need to do a little more digging, but my understanding/theory ATM is that we are relying on console/STDOUT for getting things into Splunk. I'm not 100% sure where that log sink is configured (that I need to look at). I'm going to partially confirm that in 5ae5bdb - the current version that's deployed is missing the metadata fields in it (presumably bc I put used the wrong key here). I'll deploy that momentarily, and see if that adds them back.
There was a problem hiding this comment.
Since we're sending STDOUT to Splunk, this is why we have this configuration still. I think b/w this thread and this one the original question is answered, but LMK if that's not the case
There was a problem hiding this comment.
I think my question stands, but to rephrase: Why is any change needed here at all? Why doesn't :console still work?
| @moduledoc false | ||
|
|
||
| @spec upload_takeover_image(String.t(), binary(), String.t()) :: :ok | ||
| def upload_takeover_image(_alert_id, _image_data, _image_type), do: :ok |
There was a problem hiding this comment.
I'm thinking of this almost as a warning that the calling code is untestable, because the :error branch can never be reached when calling the test version of the function, which always returns :ok. A possible solution would be to make the function return :error when a specific sentinel value is supplied as one of the arguments. (That alone would probably resolve the compiler error without actually forcing us to add test coverage of the branch, but that would be a logical next step.)
3 participants
Asana task: Update Screenplay to Elixir 1.20 / OTP 29
Description
Update elixir from 1.17.3-otp-27 to 1.20.2-otp-29. Update erlang from 27.3.4 to 29.0.2.
Version 0.18 of
fakeris incompatible with elixir 1.20, which necessitated an upgrade.Update Logger configuration & install
logger_backends, which is where the pre-existing logging backend system has moved to.Elixir's built-in
JSONmodule that was added in v1.18 replaces most usages ofJasonin the codebase.Jasonhas been left as a dependency for screens configuration saving (used for emergency takeover setting) -JSONdoes not support a pretty-printing, which is relied on to save a well-formed configuration file.:json.formatwas considered, but does not transformniltonull, whichJason.encode!()does. Given that we are looking to move this configuration to Postgres, keepJasonfor the time being, until the migration is complete and we may revisit.Reorder lists in tests whose ordering has changed as a result of changes in erlang.
Elixir v1.20 was able to detect unused
require Loggerstatements, which are removed.Add a Phoenix code reloading listener to suppress warnings that were printed to STDOUT after the upgrade when running the dev server.