Skip to content
This repository has been archived by the owner on Sep 16, 2022. It is now read-only.

fix: Update dependencies, ensure build success and add axum-middleware-example #72

Closed
wants to merge 19 commits into from

Conversation

SiddheshKanawade
Copy link
Member

@SiddheshKanawade SiddheshKanawade commented Jun 14, 2022

Planned to ensure build with dependencies updated to latest version. Also add axum-middleware-example.

TODO:

  • Update dependencies to their latest version. solved Build(deps): update actix-web requirement from 3.0 to 4.1 #71
  • Add enable github actions on pull request.
  • Both tokio and async-std features of casbin are enabled, disable one to ensure casbin compile successfully
  • Make actix-fileadapter-rbac compile successfully after updating the dependencies
  • Remove the actix component version conflict and successfully compile actix-middleware-example
  • Remove the deprecated calls after updating dependencies and successfully compile ntex-fileadapter-acl
  • Add axum-middleware-example
  • Make clippy happy

solved #71

@casbin-bot
Copy link

@smrpn @hackerchai @PsiACE @GopherJ please review

@hsluoyz
Copy link
Member

hsluoyz commented Jun 14, 2022

@hackerchai

@SiddheshKanawade
Copy link
Member Author

@hackerchai do I need to do anything else?

@hsluoyz
Copy link
Member

hsluoyz commented Jun 19, 2022

@hackerchai plz review

Copy link
Member

@hackerchai hackerchai left a comment

Choose a reason for hiding this comment

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

LGTM

@hackerchai
Copy link
Member

@SiddheshKanawade Can you also add pull request github actions to run build checks?

@SiddheshKanawade
Copy link
Member Author

@hackerchai I have added the pull_request check and also the cache, not sure the cache will work or not, last time it didn't worked

@hsluoyz
Copy link
Member

hsluoyz commented Jun 19, 2022

@SiddheshKanawade plz fix:

image

@SiddheshKanawade
Copy link
Member Author

The above commits ensures that actix-fileadapter-rbac, actix-pgsql-simple, ntex-fileadapter-acl compiles successfully.

In actix-middleware-example, there are various version conflicts that I am trying to resolve(some examples below). Any suggestions on how should I resolve them? Also, the depedencies shown for actix-casbin-auth aren't updated in crates.io, I think this might be the issue(not sure though). It says that v3 of actix-web is used and not v4.

@SiddheshKanawade
Copy link
Member Author

The possible cure here could be to match the versions of the dependencies to that of the crate.io but then it would restrict us to update the dependencies

@hackerchai
Copy link
Member

please fix

@SiddheshKanawade
Copy link
Member Author

please fix

Yeah, I am working on it, its probably due to two different versions of tokio

@SiddheshKanawade SiddheshKanawade changed the title fix: Upgraded the dependencies fix: Update dependencies, ensure build success and add axum-middleware-example Jul 15, 2022
@SiddheshKanawade
Copy link
Member Author

The crates.io don't have the versions of the dependencies updated, thus had to do following change to allow the use of newer versions of the actix dependencies without conflict
From

actix-casbin= {git = "https://github.com/casbin-rs/actix-casbin", default-features = false, features = [ "runtime-async-std" ]}
actix-casbin-auth = {git = "https://github.com/casbin-rs/actix-casbin-auth", default-features = false, features = [ "runtime-async-std" ]}

To

actix-casbin= {version = "0.4.2", default-features = false, features = [ "runtime-async-std" ]}
actix-casbin-auth = {version = "0.4.4", default-features = false, features = [ "runtime-async-std" ]}

This needs to be updated ones versions are updated in crates.io

@SiddheshKanawade SiddheshKanawade marked this pull request as draft July 16, 2022 15:50
@SiddheshKanawade
Copy link
Member Author

@hackerchai I need to get PR #25 merged, so that we have updated tokio version in actix-casbin and avoid the tokio version conflict here. Also, I have completed my work on this PR and can be reviewed once the above mentioned PR gets merged.

@hackerchai
Copy link
Member

@SiddheshKanawade plz fix the CI errors

@SiddheshKanawade SiddheshKanawade marked this pull request as ready for review August 5, 2022 16:58
@SiddheshKanawade
Copy link
Member Author

Screenshot from 2022-08-06 13-09-06

I am not getting this error in my local env, any ideas on how should I cope with it?

@SiddheshKanawade
Copy link
Member Author

@hackerchai All of the tasks have been completed and all checks are passing, kindly review and let me know if I need to do anything else

@hsluoyz
Copy link
Member

hsluoyz commented Aug 19, 2022

@SiddheshKanawade plz don't make so big and long-term PRs. You should not make just one PR for the whole summer and put everything in it. Our community's PRs are expected to be small-scale and will be reviewed & merged within 2-3 days, at most one week ideally.

@SiddheshKanawade
Copy link
Member Author

Yeah, actually this repository had a lot to do before I can make build run, so initially the PR became long. Also, this was what I was planning to do initially, but as per instructed by my mentor, I switched to axum-casbin-auth and hence this PR got extended. When I switched back to the task, rather than opening a new PR, I continued to work on this rather than closing it.
Also, your suggestion makes a lot sense and will keep this in mind in future PRs.
Thank You.

@hsluoyz
Copy link
Member

hsluoyz commented Aug 19, 2022

OK.

@hackerchai plz review and get this one merged ideally

@SiddheshKanawade
Copy link
Member Author

Since now we have refactored this repository into smaller repositories, this PR has no significance and hence closing it.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants