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

Update jose library #6

Merged

Conversation

fastjames
Copy link
Contributor

@fastjames fastjames commented Feb 20, 2024

Update jose to 1.11.x to remove :crypto calls that were deprecated in OTP23 and were removed in OTP24.

Update jose to 1.11.x to remove :crypto calls that were deprecated in
OTP23 and were removed in OTP24.
@ulissesalmeida
Copy link
Collaborator

@fastjames It seems there's error when booting Goth. Let me know if you need any help to debug it.

@fastjames
Copy link
Contributor Author

@fastjames It seems there's error when booting Goth. Let me know if you need any help to debug it.

I dug into this a little bit deeper, and there seems to be a knot of dependencies. As of 1.11.4 (the next released version after our current version of 1.10.1), jose specifies an elixir version of "~> 1.13" in its mix.exs. So, it looks like jose does not have a release that supports elixir 1.12 / OTP24. :-(

@ulissesalmeida
Copy link
Collaborator

@fastjames I think they will never add 🤣

@petermueller What do you think in dropping the Elixir 1.12 support as minor release + jose update?

Since jose does not appear to have support for elixir 1.12 as of the
1.11 series, remove it from the test matrix.
@petermueller
Copy link
Collaborator

petermueller commented Feb 21, 2024

@ulissesalmeida @fastjames I think it's fine. We should probably update our elixir version in mix.exs then as well, if you can do that too, @fastjames 🙏🏻

Generally, it's enough though that we'd probably want the next release to be 0.3.0 with an upgrade guide.

e.g.

  1. Upgrade your app to Elixir 1.13
  2. Update this library
  3. Follow the Goth instructions
  4. Copy-paste the example from (the README changes from Add official support to Goth >= 1.3 #2) into your app

Either way, I think this update makes sense, even if it's not ideal

Edit:
I'll make a separate issue for the upgrade guide if you all think a 0.3.0 bump makes sense too

Bring the elixir spec in the mixfile in line with the test matrix.
@fastjames
Copy link
Contributor Author

@ulissesalmeida @fastjames I think it's fine. We should probably update our elixir version in mix.exs then as well, if you can do that too, @fastjames 🙏🏻
I have pushed a commit to update the elixir version in mix.exs. Thanks for pointing that out!

@petermueller
Copy link
Collaborator

I think the errors are to do with however GitHub's repo secrets work

@ulissesalmeida
Copy link
Collaborator

@petermueller you're right, the secrets are only shared with branches created on this repo. Not on external repos.

@fastjames
Copy link
Contributor Author

@petermueller you're right, the secrets are only shared with branches created on this repo. Not on external repos.

That's good to know -- I will try to add the needed secrets to my repo.

@fastjames
Copy link
Contributor Author

@petermueller you're right, the secrets are only shared with branches created on this repo. Not on external repos.

That's good to know -- I will try to add the needed secrets to my repo.

I have added secrets for GCP_CREDENTIALS and WAFFLE_BUCKET to my fork, so maybe that will improve the behavior of the CI suite on my PR.

@fastjames
Copy link
Contributor Author

I opened a PR for the same code on my repo with my secrets configured, and it appears to have run CI successfully. Perhaps one of the maintainers can open a new PR with the equivalent code here and it might pass.

@ulissesalmeida
Copy link
Collaborator

@fastjames we'll merge it soon with goth upgrade, but before we'll launch a patch.

So no worries! 👍🏽

Copy link
Collaborator

@petermueller petermueller left a comment

Choose a reason for hiding this comment

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

confirmed the PR on the fork passes

@petermueller petermueller mentioned this pull request Feb 25, 2024
@petermueller petermueller merged commit cd009ea into elixir-waffle:main Apr 5, 2024
0 of 4 checks passed
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