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

create a separate crate for configuration #121

Merged
merged 6 commits into from
May 7, 2024

Conversation

pranshi06
Copy link
Collaborator

@pranshi06 pranshi06 commented May 3, 2024

What

Refactors the configuration code and create a new crate for it that doesn't depend on ndc-sdk directly.
Reason: ndc-sdk depends on reqwest crate which calls openssl which in turn creates issue when creating the aarch-linux CLI binary. There is no need for reqwest while creating the configuration for the data connector.

Doubts: I'm a little uncertain as to what errors we should be using for the configuration. I'll update the missing URI error in the PR that introduces reading configuration URI from environment.

How

@pranshi06 pranshi06 marked this pull request as ready for review May 6, 2024 16:57
Comment on lines +1 to +7
//! Errors that can be thrown when processing configuration.

/// The errors that can be thrown when processing configuration.
///
/// This is effectively a copy of the `ParseError` enum in the `ndc-sdk` crate. However, we don't
/// want a dependency on that crate here, as this crate is used by the CLI. Duplicating here and
/// converting the values later means we can avoid that dependency.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is the ndc-sdk crate introducing the indirect dependency on openssl?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, ndc-sdk depends on reqwest which in turn depends on openssl

Copy link
Collaborator

@codingkarthik codingkarthik left a comment

Choose a reason for hiding this comment

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

LGTM!

@pranshi06 pranshi06 added this pull request to the merge queue May 7, 2024
@pranshi06 pranshi06 removed this pull request from the merge queue due to a manual request May 7, 2024
@pranshi06 pranshi06 added this pull request to the merge queue May 7, 2024
Merged via the queue into main with commit 98b8575 May 7, 2024
18 checks passed
@pranshi06 pranshi06 deleted the py/configuration-crate-refactor branch May 7, 2024 13:10
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.

2 participants