Skip to content

deps(common_utils): put the signals module behind a feature flag #814

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.

Sign up for GitHub

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

Jump to bottom
Merged
jarnura merged 5 commits into from
Apr 4, 2023
Merged

deps(common_utils): put the signals module behind a feature flag #814

jarnura merged 5 commits into from
Apr 4, 2023

Conversation

@vspecky
Copy link
Contributor

@vspecky vspecky commented Mar 30, 2023

Type of Change

  • Bugfix
  • New feature
  • Enhancement
  • Refactoring
  • Dependency updates

Description

This PR puts the signals module in common_utils behind a feature flag.

Additional Changes

None

  • This PR modifies the database schema
  • This PR modifies application configuration/environment variables

Motivation and Context

Required for the api_models crate to be independent of any async/tokio related dependencies.

How did you test it?

cargo build

Checklist

  • I formatted the code cargo +nightly fmt --all
  • I addressed lints thrown by cargo clippy
  • I reviewed submitted code
  • I added unit tests for my changes where possible
  • I added a CHANGELOG entry if applicable

@vspecky vspecky added A-dependencies Area: Dependencies S-waiting-on-review Status: This PR has been implemented and needs to be reviewed C-refactor Category: Refactor labels Mar 30, 2023
@vspecky vspecky self-assigned this Mar 30, 2023
@vspecky vspecky requested a review from a team as a code owner March 30, 2023 06:02
@NishantJoshi00
Copy link
Contributor

NishantJoshi00 commented Mar 30, 2023

For this change to be more effective, can you please move this into the same file, https://github.com/juspay/orca/blob/01a5e0a0c927a9b2b5ff86b5607b0ea882674c85/crates/router/src/scheduler/utils.rs#L391-L414

This will reduce the are of change required to make the project compatible with windows machines in the future

NishantJoshi00
NishantJoshi00 reviewed Mar 30, 2023
View reviewed changes
Copy link
Contributor

@NishantJoshi00 NishantJoshi00 left a comment

Choose a reason for hiding this comment

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

Other than that, looks good to me!

@vspecky vspecky requested a review from a team as a code owner March 30, 2023 07:09
NishantJoshi00
NishantJoshi00 previously approved these changes Mar 30, 2023
View reviewed changes
Copy link
Contributor

@NishantJoshi00 NishantJoshi00 left a comment

Choose a reason for hiding this comment

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

LGTM

lsampras
lsampras previously approved these changes Mar 30, 2023
View reviewed changes
jarnura
jarnura previously approved these changes Mar 30, 2023
View reviewed changes
@jarnura jarnura removed the S-waiting-on-review Status: This PR has been implemented and needs to be reviewed label Mar 30, 2023
@jarnura jarnura added this pull request to the merge queue Mar 30, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to a conflict with the base branch Mar 30, 2023
@vspecky vspecky dismissed stale reviews from jarnura, lsampras, and NishantJoshi00 via 1c2830c March 30, 2023 10:12
@jarnura jarnura added the S-waiting-on-approval Status: Review completed, awaiting for approvals from code-owners label Mar 30, 2023
@vspecky vspecky added S-ready-for-merge S-waiting-on-review Status: This PR has been implemented and needs to be reviewed and removed S-waiting-on-approval Status: Review completed, awaiting for approvals from code-owners S-ready-for-merge labels Apr 3, 2023
@jarnura jarnura removed the S-waiting-on-review Status: This PR has been implemented and needs to be reviewed label Apr 4, 2023
@jarnura jarnura added this pull request to the merge queue Apr 4, 2023
Merged via the queue into main with commit fb4ec43 Apr 4, 2023
@Gnanasundari24 Gnanasundari24 mentioned this pull request Apr 4, 2023
Merged
@SanchithHegde SanchithHegde deleted the make-signals-optional branch April 17, 2023 19:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jarnura jarnura jarnura approved these changes

@SanchithHegde SanchithHegde Awaiting requested review from SanchithHegde

@lsampras lsampras Awaiting requested review from lsampras

+1 more reviewer

@NishantJoshi00 NishantJoshi00 NishantJoshi00 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@vspecky vspecky

Labels

A-dependencies Area: Dependencies C-refactor Category: Refactor

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@vspecky @NishantJoshi00 @jarnura @lsampras