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

Support rotp 5 plus #63

Merged
merged 6 commits into from
Jun 13, 2022

Conversation

daniel-casey
Copy link
Contributor

Updated to support the changes in rotp 5+

As rotp 6 contains potentially breaking changes (defaults to 32 character secret, drops support for Ruby <2.3) it probably merits bumping the version to 3.0

@daniel-casey
Copy link
Contributor Author

@jaredonline It looks like Travis is no longer connected, so I can't see whether this is passing or failing. Either way hopefully this is a better starting point for getting this merged.

@jaredonline
Copy link
Owner

@daniel-casey It looks like Travis no longer provides free builds for open source. I'm in the process of trying to get CircleCI to do the matrix build. Once that works, we should be able to run your branch against it.

@jaredonline
Copy link
Owner

@daniel-casey, can you push a new commit to this and see if it builds on CircleCI? You may need to merge master first.

@jaredonline
Copy link
Owner

Oh; I just noticed this is a PR against 2.0-stable; let's make it be against master and I'll cut 3.0 from that.

@daniel-casey daniel-casey changed the base branch from 2.0-stable to master June 12, 2022 22:09
@daniel-casey
Copy link
Contributor Author

@jaredonline I've rebased this onto master, resolved the conflicts and changed the PR to point to master. CircleCI doesn't seem to have run - am I missing something, or do you maybe need to enable "Build forked pull requests"? https://circleci.com/blog/triggering-trusted-ci-jobs-on-untrusted-forks/

@jaredonline
Copy link
Owner

@daniel-casey thanks for helping me troubleshoot this. I've enabled builds for forked branches. Let's see if a commit will trigger a build? If not I'll just pull your build and run it manually 🤷

@daniel-casey
Copy link
Contributor Author

@jaredonline Sweet - I push an empty commit and Circle ran successfully.

Copy link
Owner

@jaredonline jaredonline left a comment

Choose a reason for hiding this comment

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

Thanks for your patience on this!

@jaredonline jaredonline merged commit 6643fd9 into jaredonline:master Jun 13, 2022
@jaredonline
Copy link
Owner

@daniel-casey is there more that should be put in before cutting a 3.0.0 version?

@daniel-casey
Copy link
Contributor Author

@jaredonline This is looking good to me. Maybe you can add the small change from PR #62 directly rather than waiting for them to rebase? I haven't seen the issue in PR #56 myself, but maybe do the same for that one?

Once you get 3.0.0 up I'll drop it into a real project and try it out.

This was referenced Jun 15, 2022
@johan-smits
Copy link

@jaredonline can we expect a release of the gem?

@jaredonline
Copy link
Owner

@daniel-casey @johan-smits I just released 3.0. Thanks for your help and patience [=

@daniel-casey
Copy link
Contributor Author

@jaredonline the change to use ActiveSupport.on_load(:action_controller) seems to cause RailsAdapter::LoadedTooLateError when running rspec under Rails 7. The error is not raised in the same application on Rails 5.2, so possibly something to do with the switch to zeitwerk in Rails 7.

Any idea how to fix / avoid this?

@jaredonline
Copy link
Owner

@daniel-casey what version of Ruby? It's interesting that the tests passed in CircleCI

Can you push a failing test as a PR so I can look?

@jaredonline
Copy link
Owner

@daniel-casey Oh I see, you mean if you install this gem in Rails 7.0 then run rspec, the tests fail with that error. I haven't used this with Rails 7.0 before (honestly I haven't used it since Rails 3.0).

I'll poke around next week and see if I can figure it out.

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.

None yet

3 participants