-
Notifications
You must be signed in to change notification settings - Fork 82
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
Drop Elixir versions before 1.11 #953
Conversation
Utils.warning/1 and Utils.info/1 were added to support Elixir versions before 1.11. Since we're in the process of dropping support for versions before 1.11, these can be removed. This patch removes both functions and switches all calls to them to use Logger.warning/1 and Logger.info/1 directly.
In the process of dropping Elixir versions before 1.11, remove Mix.Appsignal.Utils, which was added to switch between Application.compile_env/3 and Application.get_env/3. [skip changeset]
To allow for setups running a newer version of appsignal-elixir with older versions of appsignal-phoenix or appsignal-plug, add a deprecated fallback in order to not break any apps. [changeset skip]
In the process of dropping Elixir versions before 1.11, remove Appsignal.Utils.compile_env/3, which was added to switch between Application.compile_env/3 and Application.get_env/3. [skip changeset]
We've found some issues with your Pull Request.
|
To allow for setups running a newer version of appsignal-elixir with older versions of appsignal-phoenix or appsignal-plug, add a deprecated fallback in order to not break any apps. [changeset skip]
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.
You can add a changeset about us dropping support so people don't find out after upgrading.
32300d9
to
2687df4
Compare
Instead of deprecating the no-longer-needed Appsignal.Utils.info/1, Appsignal.Utils.warning/1, and Appsignal.Utils.compile_env/3, wait until a new version of both Appsignal for Phoenix and Appsignal for Plug are released. That way, we won't run into a situation where a new version of Appsignal for Elixir is released that produces deprecation warnings, while the other libraries aren't out yet. This allows for merging this patch without having to immediately do a release.
I've decided to remove the deprecation warnings for now (3457f08), to allow us to merge this patch without having to immediately having to release it. We'll add a deprecation warning later, after a new version of the AppSignal for Phoenix and Plug libraries have been released. |
This comment has been minimized.
This comment has been minimized.
2 similar comments
This comment has been minimized.
This comment has been minimized.
This is a message from the daily scheduled checks. |
AppSignal for Elixir supports the Elixir and OTP versions listed on the compatibility page. Currently, that's Elixir 1.13 and up, and therefore OTP 25 and up.
However, we don't tend to remove old versions from CI until they start giving us issues. When switching the CI to GitHub actions, we found that we could no longer install OTP versions lower than 24, as they're incompatible with the Ubuntu version we're running. Instead of trying to compile from source, we decided to drop Elixir 1.9.x and 1.10.x. Currently, the CI runs Elixir 1.11 and up, and OTP 24 and up.
By dropping these versions, we were able to remove some code we added for compatibility with versions before Elixir 1.11:
Appsignal.Utils.warning/1
was added to bridge the gap betweenLogger.warn/1
andLogger.warning/1
, after the rename in 1.11. Since we don't support versions before 1.11 anymore, we don't need the helper function and we can just callLogger.warning/1
directly.Appsignal.Utils.compile_env/3
is a similar case, in whichApplication.compile_env/3
was introduced in Elixir 1.10. The helper dropped down toApplication.get_env/3
on older versions. We can now callApplication.compile_env/3
directly.Both of these functions were also used in AppSignal for Phoenix and AppSignal for Plug. Pull requests are opened for both (appsignal/appsignal-elixir-phoenix#94 and appsignal/appsignal-elixir-plug#47).
Because this pull request includes fallbacks, we don't need to release these in a specific order. In unlikely cases where a user updates AppSignal for Elixir without updating Appsignal for Phoenix, the integration will remain working, but it will throw deprecation warnings.