Skip to content
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

Check absinthe dependent libraries on CI runs #1322

Merged
merged 1 commit into from
Jun 14, 2024
Merged

Conversation

bryanjos
Copy link
Contributor

This PR adds a GitHub action to check the libraries that depend on Absinthe to ensure they still work with the changes added in PRs.

@benwilson512
Copy link
Contributor

Oh this reminds me, we should update the Elixir versions we test against, particularly with 1.17 out. We should only do 1.15, 1.16, and 1.17

- elixir: "1.15"
otp: "26"
format: true
- '24'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm fine dropping this one at this point.

@maartenvanvliet
Copy link
Contributor

Is it failing because of the change/fix in the String.jaro_distance/2 in Elixir? elixir-lang/elixir#13369

@bryanjos
Copy link
Contributor Author

@maartenvanvliet thanks for that! I've been trying to figure it out. Especially since it passes locally with Elixir 1.17 and OTP 27, though I'm on a mac. I'll look and see if that's the issue.

@bryanjos
Copy link
Contributor Author

I guess I wasn't running those versions locally after all. Updating, I can now see the failed tests locally. I'll see if I should update the test or the jaro distance threshold.

* Update elixir versions in CI to test 1.15, 1.16, and 1.17
* Update OTP version in CI to test 25, 26, and 27
* Update ubuntu version to 24.04
* Lower jaro_distance threshold slightly
* Remove else part of with expression in generate_schema. Dialyzer
  saw that Absinthe.Schema.introspect will only return
  {:error, String.t()} if there is an error
@bryanjos
Copy link
Contributor Author

@benwilson512 I think this is good to go now! Aside from the CI changes, I lowered the jaro_distance slightly (thanks again @maartenvanvliet!), and fixed an issue dialyzer found.

Copy link
Contributor

@benwilson512 benwilson512 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really love how clean this is, thanks!

@maartenvanvliet
Copy link
Contributor

@benwilson512 I think this is good to go now! Aside from the CI changes, I lowered the jaro_distance slightly (thanks again @maartenvanvliet!), and fixed an issue dialyzer found.

No problem, saw the CI error and vaguely recalled the change in elixir a few months back

@bryanjos bryanjos merged commit 66ccfee into main Jun 14, 2024
9 checks passed
@bryanjos bryanjos deleted the bryanj/check_dependents branch June 14, 2024 18:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants