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

Upgrade axum to 0.7.4 #121

Merged
merged 11 commits into from Feb 23, 2024
Merged

Upgrade axum to 0.7.4 #121

merged 11 commits into from Feb 23, 2024

Conversation

cscetbon
Copy link
Contributor

@cscetbon cscetbon commented Feb 18, 2024

Including in this PR:

  • Upgrade of axum from 0.6.19 to 0.7.4
  • Upgrade of dependencies that depend on axum
  • Upgrade of env_logger crate
  • Export axum from dapr::server::actor
  • Use absolute paths in actor proc macro to use axum from dapr and not from the client
  • Upgrade of toolchain as it was failing at resolving dependency/feature of env_logger

Signed-off-by: Cyril Scetbon <cscetbon@gmail.com>
Signed-off-by: Cyril Scetbon <cscetbon@gmail.com>
@cscetbon cscetbon marked this pull request as ready for review February 18, 2024 03:50
@cscetbon cscetbon force-pushed the upgrade-axum branch 2 times, most recently from 9f2f5ec to 17280bd Compare February 18, 2024 13:35
Signed-off-by: Cyril Scetbon <cscetbon@gmail.com>
Signed-off-by: Cyril Scetbon <cscetbon@gmail.com>
Signed-off-by: Cyril Scetbon <cscetbon@gmail.com>
Signed-off-by: Cyril Scetbon <cscetbon@gmail.com>
Signed-off-by: Cyril Scetbon <cscetbon@gmail.com>
Signed-off-by: Cyril Scetbon <cscetbon@gmail.com>
Signed-off-by: Cyril Scetbon <cscetbon@gmail.com>
Copy link
Member

@mikeee mikeee left a comment

Choose a reason for hiding this comment

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

First look

src/server/actor/tests.rs Show resolved Hide resolved
src/server/actor/tests.rs Outdated Show resolved Hide resolved
src/server/actor/tests.rs Show resolved Hide resolved
src/server/utils.rs Show resolved Hide resolved
Signed-off-by: Cyril Scetbon <cscetbon@gmail.com>
Signed-off-by: Cyril Scetbon <cscetbon@gmail.com>
@cscetbon
Copy link
Contributor Author

@yaron2 Can you take a look ?

@yaron2
Copy link
Member

yaron2 commented Feb 21, 2024

@yaron2 Can you take a look ?

LGTM, waiting on final approval from @mikeee.

FYI it would be extremely useful to add the ability to specify the Dapr endpoint to override 127.0.0.1 the same way other SDKs are doing through environment variables.

Copy link
Member

@mikeee mikeee left a comment

Choose a reason for hiding this comment

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

LGTM! The upgrade path makes sense, thanks for the clarifications.

@cscetbon
Copy link
Contributor Author

cscetbon commented Feb 22, 2024

FYI it would be extremely useful to add the ability to specify the Dapr endpoint to override 127.0.0.1 the same way other SDKs are doing through environment variables.

@yaron2 Let's do it in another PR, but it would need a good description cause I've read that the dapr side car currently does not support invoking actors via grpc, and other SDKs use an env variable called DAPR_GRPC_ENDPOINT.

That PR got a 👍 from both of you so it should be ready to merge 😉

@yaron2 yaron2 merged commit bef6e3b into dapr:master Feb 23, 2024
5 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.

None yet

3 participants