Implement configuration #15

Kirguir · 2019-01-31T13:13:06Z

Implement a layer that implements the configuration of the application and provides the means to work with it.

Kirguir · 2019-01-31T13:31:53Z

@alexlapa напишите, пожалуйста, какие параметры нужны в конфиге, как будут задаваться (файл, env) и значения по-умолчанию, если нужны.

alexlapa · 2019-01-31T17:34:13Z

@Kirguir ,

В сорцах стоят дефолтные значения, переопределяются они через конфигурационный файл или через переменные среды. А вот по приоритету config / env var уже не так очевидно. Я бы вообще просто панику бросал, если одна и та-же переменная прокидывается и через конфиг и через окружение и с разными значениями. Как в SAPI было сделано? env var > config?

По поводу конкретных параметров сейчас мудрить смысла не вижу - они в процессе сами будут появляться. Например, HEARTBEAT_INTERVAL и CLIENT_TIMEOUT из #15 .

# Conflicts: # Cargo.lock # Cargo.toml # src/main.rs

…me, add basic makefile, add config tests, add duration serde tests

alexlapa · 2019-03-26T15:48:52Z

@tyranron ,

Гляньте, пожалуйста, будут ли у Вас замечания по реализации. Документацию добавлю после, и тогда уже попрошу полное ревью.

tyranron

The overall impression that code may be simplified.

Consider to use structopt for command line.

Wrapping into untyped config::Config for merging is also a bit strange. Can't this be achieved without that?

tyranron · 2019-03-29T14:45:25Z

Cargo.toml

 futures = "0.1"
 hashbrown = "0.1"
+humantime = "1.2.0"


There is no sense to specify patch version due to work of default carret requirement.

Just humantime = "1.2" is enough.

tyranron · 2019-03-29T14:50:35Z

src/conf/mod.rs

+};
+use failure::Error;
+use serde_derive::{Deserialize, Serialize};
+use std::collections::HashMap;


Do not forget to separate std imports .

tyranron · 2019-03-29T14:51:42Z

src/conf/server.rs

+    fn default() -> Self {
+        Self {
+            bind_ip: IpAddr::V4(Ipv4Addr::new(0, 0, 0, 0)),
+            bind_port: 8080,


Consider to use smart-default. It's way more ergonomic.

tyranron · 2019-03-29T14:53:56Z

src/conf/server.rs

+
+impl Server {
+    pub fn get_bind_addr(&self) -> impl std::net::ToSocketAddrs {
+        (self.bind_ip, self.bind_port)


Why returning impl? It has no sense in this context. It's almost always to return a concrete type rather then it's abstraction. So, just (IpAddr, u16) is enough.

@tyranron ,

Если можно вернуть интерфейс - нужно возвращать интерфейс. Просто хороший тон. Но, не принципиально, на самом деле.

@Kirguir why this is a good tone? It just erases additional type information. "Expect wider type, but return concrete type".

@tyranron ,

Абстракции, инкапсуляция, loose coupling, все дела.

@alexlapa hmm... I agree. I this context it allows to hide the inner representation of the Server, which makes sense in the usage places where we require just ToSocketAddrs.

tyranron · 2019-03-29T14:59:17Z

Cargo.toml

 serde = { version = "1.0", features = ["derive"] }
+serde_derive = "1.0"


Why add serde_derive again when the next line above contains features = ["derive"]?

tyranron · 2019-03-29T15:06:39Z

src/conf/duration.rs

+        E: de::Error,
+    {
+        let dur = humantime::parse_duration(value).map_err(|_| {
+            de::Error::invalid_value(Unexpected::Str(value), &self)


Wow, this seems tough. This should be implemented in some util module, or even a separate crate.

Fortunately, reading documentation carefully allows to avoid writing redudant code: serde-humantime 🙃

@tyranron ,

Прикрутил, но там только deserialize. А serialize используется в этом финте, который там не от хорошей жизни. Получится выкосить его - выкошу serialize.

@alexlapa I think we should anyway have Serialize capability, even for debug purposes, like "export me the current config".

A bit more exploring and we have this.

tyranron · 2019-03-29T15:11:50Z

src/conf/mod.rs

+    /// - configuration file, the name of which is given as a command line
+    /// parameter or environment variable;
+    /// - environment variables;
+    pub fn new() -> Result<Self, Error> {


I guess parse() is a better name here.

tyranron · 2019-03-29T15:17:18Z

src/api/client/server.rs

    };

    use super::*;
+    use crate::conf::server::Server;


This is not very ergonomic due to know how conf module is layouted. It's better to re-export stuff in conf, so I can just do conff::Server.

alexlapa · 2019-03-29T20:32:08Z

@tyranron ,

Вы, помнится, заметили, что, вроде бы, вот этот кусок:

    let mut cfg = Config::new();
    cfg.merge(Self::default())?;
    let s: Self = cfg.try_into()?;

можно реализовать корректнее. Напомните, пожалуйста, каким образом.

tyranron · 2019-03-30T12:35:36Z

@alexlapa nope, I've investigated a config crate and its examples a bit further, and it's seems that this the idiomatic and correct way. I think my confusion goes from Go experience.

However, as far as I found, we may omit config::Source implementation by doing:

let mut cfg = Config::try_from(&Self::default())?;

which requires only Serialize on T.

alexlapa · 2019-04-01T13:35:16Z

@tyranron ,

let mut cfg = Config::try_from(&Self::default())?;

А это то, о чем говорил Kirguir . Там, в нутре config::Config, есть две HashMap'ы: defaults и overrides. Config::try_from пишет в overrides и последующие Config::merge эти значения не обновляют.

alexlapa · 2019-04-01T17:53:55Z

@tyranron ,

Изучил structopt. Без дополнительной обработки напильником structopt и config-rs не дружат. structopt умеет arg, env vars. config-rs умеет в file, env vars. Если мы хотим чтобы args были наиболее приоритетными (args>env>file>default), то мержить надо ручками. Предлагаю просто дождаться пока config-rs интегрируют clap, issue тут. А пока сможем и без передачи конфига через args пожить. Ну или можно сделать (file>args>env>>default), что, в общем, тоже не плохо.

tyranron · 2019-04-02T11:11:17Z

@alexlapa

А это то, о чем говорил Kirguir . Там, в нутре config::Config, есть две HashMap'ы: defaults и overrides. Config::try_from пишет в overrides и последующие Config::merge эти значения не обновляют.

meh... 😕

OK, now the current implementation is OK. Later, I'll fix the issue in config-rs when will mess with it in my project.

Изучил structopt. Без дополнительной обработки напильником structopt и config-rs не дружат. structopt умеет arg, env vars. config-rs умеет в file, env vars. Если мы хотим чтобы args были наиболее приоритетными (args>env>file>default), то мержить надо ручками. Предлагаю просто дождаться пока config-rs интегрируют clap, issue тут. А пока сможем и без передачи конфига через args пожить. Ну или можно сделать (file>args>env>>default), что, в общем, тоже не плохо.

Agree, for now we can skip it. Changing the priorities i not the solution.

tyranron

A little refactoring is done to simplify code more and make tests more BDD'ish.
Removed unused dependencies.
Refactored Makefile to throw away Docker squats.

Do not forget about FCM.

FCM:

Create application configuration (#15)

- impl conf module which holds typed configuration of application
- parse configuration from file and env vars
- allow to specify configuration file path
- refactor existing code to use values from configuration options

Add dependencies

f669e41

Kirguir self-assigned this Jan 31, 2019

Kirguir added feature New feature or request k::design Related to overall design and/or architecture k::config Related to application configuration labels Jan 31, 2019

Kirguir added this to the 0.1.0 milestone Jan 31, 2019

alexlapa mentioned this pull request Jan 31, 2019

Medea server 1 <=> 1 WebRTC P2P #10

Closed

16 tasks

Kirguir added 4 commits February 6, 2019 15:37

Impl settings

3301284

Add docs

4b0a60b

Merge branch 'master' into add_config

7b37a1d

# Conflicts: # Cargo.lock # Cargo.toml # src/main.rs

Add docs

0b454ca

Kirguir requested a review from alexlapa March 22, 2019 14:42

refactor, pass conf file path in args/env, serde duration via humanti…

754ab25

…me, add basic makefile, add config tests, add duration serde tests

alexlapa changed the title ~~WIP: Implements configuration~~ WIP: Implement configuration Mar 25, 2019

Kirguir and others added 2 commits March 26, 2019 11:16

Add docs

29461ba

refactor, extend tests

41b33ee

alexlapa requested a review from tyranron March 29, 2019 13:25

tyranron suggested changes Mar 29, 2019

View reviewed changes

tyranron and others added 6 commits March 29, 2019 17:39

Some corrections

0878ebb

fix serde_derive import

3a959af

use smart_default

7596c79

use serde-humantime

e14e091

reexports in conf::Conf

463c6a9

make clippy happy

548b463

use serde-humantime fork

66ec28b

alexlapa added 2 commits April 1, 2019 21:03

refactor

0c03deb

quickfix

a311413

alexlapa added 3 commits April 2, 2019 16:11

revert not relevant changes, update docs

72b814a

revert not relevant changes, update docs

880af87

makefile formatting fix

a9f3264

alexlapa approved these changes Apr 2, 2019

View reviewed changes

alexlapa requested a review from tyranron April 2, 2019 14:15

Corrections

4e77125

tyranron approved these changes Apr 5, 2019

View reviewed changes

Kirguir changed the title ~~WIP: Implement configuration~~ Implement configuration Apr 8, 2019

Kirguir merged commit a004d67 into master Apr 8, 2019

Kirguir deleted the add_config branch April 8, 2019 07:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement configuration #15

Implement configuration #15

Kirguir commented Jan 31, 2019 •

edited

Loading

Kirguir commented Jan 31, 2019

alexlapa commented Jan 31, 2019

alexlapa commented Mar 26, 2019

tyranron left a comment

tyranron Mar 29, 2019

tyranron Mar 29, 2019

tyranron Mar 29, 2019

tyranron Mar 29, 2019

alexlapa Mar 29, 2019

tyranron Mar 29, 2019

alexlapa Mar 29, 2019

tyranron Mar 30, 2019

tyranron Mar 29, 2019

tyranron Mar 29, 2019

alexlapa Mar 29, 2019

tyranron Mar 30, 2019

tyranron Mar 29, 2019

tyranron Mar 29, 2019

alexlapa commented Mar 29, 2019

tyranron commented Mar 30, 2019 •

edited

Loading

alexlapa commented Apr 1, 2019 •

edited

Loading

alexlapa commented Apr 1, 2019 •

edited

Loading

tyranron commented Apr 2, 2019

tyranron left a comment

		serde = { version = "1.0", features = ["derive"] }
		serde_derive = "1.0"

Implement configuration #15

Implement configuration #15

Conversation

Kirguir commented Jan 31, 2019 • edited Loading

Kirguir commented Jan 31, 2019

alexlapa commented Jan 31, 2019

alexlapa commented Mar 26, 2019

tyranron left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexlapa commented Mar 29, 2019

tyranron commented Mar 30, 2019 • edited Loading

alexlapa commented Apr 1, 2019 • edited Loading

alexlapa commented Apr 1, 2019 • edited Loading

tyranron commented Apr 2, 2019

tyranron left a comment

Choose a reason for hiding this comment

Kirguir commented Jan 31, 2019 •

edited

Loading

tyranron commented Mar 30, 2019 •

edited

Loading

alexlapa commented Apr 1, 2019 •

edited

Loading

alexlapa commented Apr 1, 2019 •

edited

Loading