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

RFC-3197: Config #3197

Merged
merged 10 commits into from
Oct 8, 2023
Merged

RFC-3197: Config #3197

merged 10 commits into from
Oct 8, 2023

Conversation

Xuanwo
Copy link
Member

@Xuanwo Xuanwo commented Sep 27, 2023

Signed-off-by: Xuanwo <[email protected]>
Signed-off-by: Xuanwo <[email protected]>
@Xuanwo Xuanwo changed the title rfc: Config RFC-3197: Config Sep 27, 2023
Signed-off-by: Xuanwo <[email protected]>
Signed-off-by: Xuanwo <[email protected]>
Signed-off-by: Xuanwo <[email protected]>
@xyjixyjixyji
Copy link
Contributor

xyjixyjixyji commented Sep 29, 2023

Just a thought, what about add a interface in trait Builder

fn config(&self) -> Box<dyn Config>

And let Config itself be

trait Config: Serialize + Deserialize + ....

This makes it more maintainable and less error prone

For example S3

impl Config for S3Config {
...
}

and

impl Builder for S3Builder {
    fn config(&self) -> Box<dyn Config> {
        self.config.boxed()
   }
}

core/src/docs/rfcs/3197_config.md Show resolved Hide resolved
@oowl oowl force-pushed the rfc-services-config branch from 6f7e61f to b7edbfe Compare September 30, 2023 13:28
@Xuanwo
Copy link
Member Author

Xuanwo commented Sep 30, 2023

trait Config: Serialize + Deserialize + ....

Providing a trait that allows Serialize + Deserialize is different from providing structs/enums that supports Serialize + Deserialize. The structs/enums allows users to work directly with detailed configuration.

Users can implement TryFrom/TryInto according to their own needs.

This is crucial for production applications as they do not want to be impacted by OpenDAL's breaking changes. And they should not.

@Xuanwo
Copy link
Member Author

Xuanwo commented Sep 30, 2023

Hi, @oowl, please don't force push my branches. Thanks.

@Xuanwo Xuanwo force-pushed the rfc-services-config branch from b7edbfe to b984a82 Compare October 1, 2023 11:05
Signed-off-by: Xuanwo <[email protected]>
@damooo
Copy link
Contributor

damooo commented Oct 2, 2023

For dealing with secrets, there is secrecy

@Xuanwo Xuanwo requested review from suyanhanx and ClSlaid October 5, 2023 12:58
@Xuanwo
Copy link
Member Author

Xuanwo commented Oct 7, 2023

For dealing with secrets, there is secrecy

Thank you for the advice. However, as stated in the proposal, this does not include anything related to secrets.

@Xuanwo
Copy link
Member Author

Xuanwo commented Oct 7, 2023

cc @suyanhanx @Ji-Xinyou @ClSlaid would you like to review again?

Copy link
Contributor

@xyjixyjixyji xyjixyjixyji left a comment

Choose a reason for hiding this comment

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

LGTM now

@Xuanwo
Copy link
Member Author

Xuanwo commented Oct 8, 2023

Hi, @suyanhanx, do you have any review comments to address?

@Xuanwo Xuanwo mentioned this pull request Oct 8, 2023
47 tasks
Copy link
Member

@suyanhanx suyanhanx left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks.

@Xuanwo Xuanwo merged commit 92be822 into main Oct 8, 2023
@Xuanwo Xuanwo deleted the rfc-services-config branch October 8, 2023 06:15
Zheaoli pushed a commit to Zheaoli/opendal that referenced this pull request Oct 12, 2023
* rfc: Config

Signed-off-by: Xuanwo <[email protected]>

* Assign numbder

Signed-off-by: Xuanwo <[email protected]>

* Fix build

Signed-off-by: Xuanwo <[email protected]>

* Add example

Signed-off-by: Xuanwo <[email protected]>

* Address comments

Signed-off-by: Xuanwo <[email protected]>

* Update

Signed-off-by: Xuanwo <[email protected]>

* Add tracking issue links

Signed-off-by: Xuanwo <[email protected]>

---------

Signed-off-by: Xuanwo <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
6 participants