Support to Elixir 1.14 #31

Efesto · 2022-10-07T14:30:44Z

Adds Elixir 1.14 and OTP 25 to CI matrix
Resolves Elixir 1.14 deprecation warnings
Updates dependencies

``` dialyxir 1.0.0 => 1.2.0 certifi 2.5.3 => 2.9.0 excoveralls 0.14.0 => 0.15.0 (minor) hackney 1.17.0 => 1.18.1 jason 1.2.2 => 1.4.0 ```

- Uses Kernel.apply/3 for `Application.get_env/3` so to not raise a warning with Elixir 1.14 - Replaces `use Bitwise` with `import Bitwise`

dvic · 2022-11-08T20:55:18Z

Any chance to get this merged?

ajvondrak · 2022-11-10T01:05:31Z

Sorry for the hold-up, I've been rather busy lately. But yes, this is on my radar. Will review Soon™.

ajvondrak

Thanks for taking the time to open this PR, @Efesto! I don't really stay on top of version compatibility like I should, so I rely on reports like this to know when the latest Elixir has caused issues. 😅

There are a bunch of subtle details to an update like this, though. (Some of which are just my own rules that live in my head, to be fair.) If you have the time to address them, I left some comments. If not, I can also patch this myself. Let me know either way!

If you have even more time than that, I'm noticing that when I run the integration tests locally (with mix integrate), I'm getting an awful lot of compiler warnings. The tests still pass, mercifully, but I'd like to clean them up as much as possible. I assume it's mostly that they need their dependencies bumped, especially in light of the new minimum version of Elixir.

.github/workflows/build.yml

.tool-versions

lib/remote_ip/block.ex

lib/remote_ip/debugger.ex

mix.lock

* Documents approach * Removes Elixir versions < 1.10 * Adds every Elixir version > 1.11 * Adds 1.14 <> OTP 23

ajvondrak

Changes look good! Just a couple minor suggestions.

Apparently I need to manually permit CI runs, and now it looks like they're failing at the mix format step, oops. So I guess that'll need to be updated. I'll try and keep up with hitting the "approve and run" button til we get some green check marks.

Materially, though, I think you've hit all the relevant points on this PR. So once we get CI passing, we should be good to merge. Thanks so much! 🙏

ajvondrak · 2022-11-15T19:34:06Z

.github/workflows/build.yml

+# The approach to the CI matrix is as following:
+# * The minimum version of elixir is the minimum supported (see https://hexdocs.pm/elixir/compatibility-and-deprecations.html)
+# * Each Elixir version is paired with the lowest supported OTP version
+# * The latest Elixir version is paired also with the highest supported OTP version
+


Great! 👍

ajvondrak · 2022-11-15T19:35:26Z

integration/tests/custom/config/config.exs

@@ -1,5 +1,4 @@
-# TODO: `import Config` when our minimum supported Elixir is ~> 1.9
-use Mix.Config
+import Config


Nice! I forgot all about this. 😂

ajvondrak · 2022-11-15T19:36:52Z

mix.exs

@@ -5,7 +5,7 @@ defmodule RemoteIp.Mixfile do
    [
      app: :remote_ip,
      version: "1.0.0",
-      elixir: "~> 1.7",
+      elixir: "~> 1.10",


Cool!

Still to do: audit the deps. I can do that separately from this PR, but just making the note to myself. :)

makes sense, for that could work to add mix audit to the CI pipeline but I agree that is something that could go in another PR :)

Not a bad idea, although mix hex.audit isn't quite what I had in mind. I think the sort of checking I do still requires some manual intervention.

Since I had a moment to check:

combine is still at v0.10.0, so no need to bump it: https://hex.pm/packages/combine

plug v1.8 is the earliest version still receiving security patches: https://github.com/elixir-plug/plug#supported-versions

however, I'd previously bumped plug to ~> 1.10 because that was the earliest version to support the new minimum elixir version: 27a108f

it looks like the earliest version of plug that required elixir ~> 1.10 is actually the latest one, v1.14 😱 https://github.com/elixir-plug/plug/blob/main/CHANGELOG.md#v1140-2022-10-31

so I guess we should bump the plug dependency to ~> 1.14 if we're targeting elixir 1.10+; makes me a bit nervous, but hopefully people aren't hard-locked to a minor version for some reason 😕

The dev/ci dependencies look like they might be experiencing some errors of their own in CI. It's less important that they have the minimum possible versions, but I would perform a similar pass on those.

I can go ahead and merge this without you having to figure out those CI failures, though, unless you want to take a crack at them (and wait for me to hit the button so CI runs 😅). Seems like the sort of work that will just be mashed into the rest of the dependency bumps.

Oh, literally did not read the thing you linked to. The mix_audit package looks pretty sweet! 😀 I'll have to check it out some more.

.github/workflows/build.yml

feedback addressed

* Updates coveralls to 1.15 and dependencies * Adds a test to remote_ip for achieving again 100% test coverage

Efesto · 2022-11-16T08:58:28Z

Seems like the problem with the CI is related to coveralls, I've updated it to 1.15 and wrote a test for fixing the coverage (it dropped to 92.5% because line 290 in remote_ip.ex wasn't tested)

Hopefully this should be the good run 😅

ajvondrak · 2022-11-16T16:04:28Z

(it dropped to 92.5% because line 290 in remote_ip.ex wasn't tested)

For posterity, that's the error clause of this function:

remote_ip/lib/remote_ip.ex

Lines 288 to 293 in 4826fc4

    
           defp add_metadata(remote_ip) do 
        
             case :inet.ntoa(remote_ip) do 
        
               {:error, _} -> :ok 
        
               ip -> Logger.metadata(remote_ip: to_string(ip)) 
        
             end 
        
           end

But: hah, Interesting! I guess it technically should be unreachable code, since invalid IPs are (theoretically) caught upstream both when pulling from the headers and when the plug actually gets invoked in an actual running web server. But the test that covers it should do the trick - and it's prettier than littering the code with coveralls-ignore lines. 👍

ajvondrak

All green! 🎉 🚢 🚀 🤩 💯 ✨ 😤 👍

Thanks so much for all the work here, even given the not-so-continuous integration. 😂

ajvondrak · 2022-11-16T16:11:52Z

mix.lock

  "combine": {:hex, :combine, "0.10.0", "eff8224eeb56498a2af13011d142c5e7997a80c8f5b97c499f84c841032e429f", [:mix], [], "hexpm", "1b1dbc1790073076580d0d1d64e42eae2366583e7aecd455d1215b0d16f2451b"},
  "dialyxir": {:hex, :dialyxir, "1.0.0", "6a1fa629f7881a9f5aaf3a78f094b2a51a0357c843871b8bc98824e7342d00a5", [:mix], [{:erlex, ">= 0.2.6", [hex: :erlex, repo: "hexpm", optional: false]}], "hexpm", "aeb06588145fac14ca08d8061a142d52753dbc2cf7f0d00fc1013f53f8654654"},
  "earmark_parser": {:hex, :earmark_parser, "1.4.12", "b245e875ec0a311a342320da0551da407d9d2b65d98f7a9597ae078615af3449", [:mix], [], "hexpm", "711e2cc4d64abb7d566d43f54b78f7dc129308a63bc103fbd88550d2174b3160"},
  "erlex": {:hex, :erlex, "0.2.6", "c7987d15e899c7a2f34f5420d2a2ea0d659682c06ac607572df55a43753aa12e", [:mix], [], "hexpm", "2ed2e25711feb44d52b17d2780eabf998452f6efda104877a3881c2f8c0c0c75"},
  "ex_doc": {:hex, :ex_doc, "0.22.6", "0fb1e09a3e8b69af0ae94c8b4e4df36995d8c88d5ec7dbd35617929144b62c00", [:mix], [{:earmark_parser, "~> 1.4.0", [hex: :earmark_parser, repo: "hexpm", optional: false]}, {:makeup_elixir, "~> 0.14", [hex: :makeup_elixir, repo: "hexpm", optional: false]}], "hexpm", "1e0aceda15faf71f1b0983165e6e7313be628a460e22a031e32913b98edbd638"},
-  "excoveralls": {:hex, :excoveralls, "0.14.0", "4b562d2acd87def01a3d1621e40037fdbf99f495ed3a8570dfcf1ab24e15f76d", [:mix], [{:hackney, "~> 1.16", [hex: :hackney, repo: "hexpm", optional: false]}, {:jason, "~> 1.0", [hex: :jason, repo: "hexpm", optional: false]}], "hexpm", "94f17478b0cca020bcd85ce7eafea82d2856f7ed022be777734a2f864d36091a"},


Oh wow, that's the first I've paid attention to this version. The excoveralls README still says to use ~> 0.10, so that's what I put in the mix.exs circa d65f851, but it was already ahead by so many versions. 😅 Good to know, thanks for keeping the mix.exs up to date! 💯

Efesto · 2022-11-16T16:24:05Z

All green! 🎉 🚢 🚀 🤩 💯 ✨ 😤 👍

Thanks so much for all the work here, even given the not-so-continuous integration. 😂

Thank you for the patience, it was a pleasure and I've learned quite some from the experience :)

- combine still sits at 0.10, no update needed. - The earliest version of plug to require Elixir 1.10+ is 1.14, which also happens to be the latest version, but I guess that's fine. - The latest ex_doc is 0.29, but it requires Elixir 1.11+ starting at 0.28. I think docs are only ever generated locally when doing a `mix hex.publish`, so we _should_ be on Elixir 1.14 (per the .tool-versions). To split the difference for local dev, I'm keeping the mix.exs at ~> 0.27, but the mix.lock at 0.29 since I'm running the latest Elixir anyway. - Latest dialyxir is 1.2, still says it supports Elixir >= 1.6, so may as well use the latest. - excoveralls was already updated in #31.

Marco Polita added 4 commits October 7, 2022 11:20

Update tool-versions to elixir 1.14 and otp 25

69e6004

Add elixir 1.14 and otp 25 to CI matrix

d693ec0

Update dependencies

1e49730

``` dialyxir 1.0.0 => 1.2.0 certifi 2.5.3 => 2.9.0 excoveralls 0.14.0 => 0.15.0 (minor) hackney 1.17.0 => 1.18.1 jason 1.2.2 => 1.4.0 ```

Resolves compilation warnings

c85c6bc

- Uses Kernel.apply/3 for `Application.get_env/3` so to not raise a warning with Elixir 1.14 - Replaces `use Bitwise` with `import Bitwise`

ajvondrak previously requested changes Nov 15, 2022

View reviewed changes

Marco Polita added 6 commits November 15, 2022 09:26

Bumps min elixir version to 1.10

284ad2f

Updates .tool-versions

367476f

replaces bitwise ~~~ with bnot

a5a614c

Updates CI matrix

ca9cb2c

* Documents approach * Removes Elixir versions < 1.10 * Adds every Elixir version > 1.11 * Adds 1.14 <> OTP 23

Reverts mix.lock file

a11b110

Removing minor limit to elixir 1.11 from ci matrix

8349fe2

Efesto requested a review from ajvondrak November 15, 2022 15:28

Updates integration tests to elixir 1.10

107347d

ajvondrak reviewed Nov 15, 2022

View reviewed changes

Marco Polita added 3 commits November 15, 2022 21:03

runs mix format

764270d

Addressing comment consistency and trailing whitespace

bd200fd

Updates coveralls

2584820

* Updates coveralls to 1.15 and dependencies * Adds a test to remote_ip for achieving again 100% test coverage

ajvondrak approved these changes Nov 16, 2022

View reviewed changes

ajvondrak merged commit 803d487 into ajvondrak:main Nov 16, 2022

Efesto deleted the elixir-1.14 branch November 16, 2022 16:24

ajvondrak mentioned this pull request Jan 6, 2024

Update Elixir on CI #34

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support to Elixir 1.14 #31

Support to Elixir 1.14 #31

Efesto commented Oct 7, 2022

dvic commented Nov 8, 2022

ajvondrak commented Nov 10, 2022

ajvondrak left a comment

ajvondrak left a comment

ajvondrak Nov 15, 2022

ajvondrak Nov 15, 2022

ajvondrak Nov 15, 2022

Efesto Nov 15, 2022

ajvondrak Nov 16, 2022

ajvondrak Nov 16, 2022

Efesto commented Nov 16, 2022

ajvondrak commented Nov 16, 2022

ajvondrak left a comment

ajvondrak Nov 16, 2022

Efesto commented Nov 16, 2022

Support to Elixir 1.14 #31

Support to Elixir 1.14 #31

Conversation

Efesto commented Oct 7, 2022

dvic commented Nov 8, 2022

ajvondrak commented Nov 10, 2022

ajvondrak left a comment

Choose a reason for hiding this comment

ajvondrak left a comment

Choose a reason for hiding this comment

ajvondrak Nov 15, 2022

Choose a reason for hiding this comment

ajvondrak Nov 15, 2022

Choose a reason for hiding this comment

ajvondrak Nov 15, 2022

Choose a reason for hiding this comment

Efesto Nov 15, 2022

Choose a reason for hiding this comment

ajvondrak Nov 16, 2022

Choose a reason for hiding this comment

ajvondrak Nov 16, 2022

Choose a reason for hiding this comment

Efesto commented Nov 16, 2022

ajvondrak commented Nov 16, 2022

ajvondrak left a comment

Choose a reason for hiding this comment

ajvondrak Nov 16, 2022

Choose a reason for hiding this comment

Efesto commented Nov 16, 2022