Switch config to YAML #239

artem-zinnatullin · 2018-11-26T07:43:40Z

Closes #225, part of #213

This change switches config format that used to be some sort of .ini file to YAML as discussed in #225

Example of .mainframer/config.yml:

remote:
  host: "computer1"
  user: "user1" # Not implemented yet.
  port: 1234    # Not implemented yet.
push:
  compression: 5
pull:
  compression: 2

Code is also getting ready for config files merging #213.

IntermediateConfig represents one of possibly multiple config files, thus everything is optional.
Config represents finalized config, thus only actually optional fields are optional

Note that remote.user and remote.port are not implemented yet (we continue to rely on ~/.ssh/config read by ssh for now)

Description was updated to reflect changes after review, you can see edit history.

arturdryomov · 2018-11-28T14:17:23Z

src/intermediate_config.rs

+}
+
+#[derive(Debug, Eq, PartialEq)]
+pub struct IntermediateCompression {


Shouldn’t we use the same value for both channels? I’m already forgetting what is the benefit of having different ones. Yes, it can be a bit faster to send sources uncompressed, but I believe compression does not eat a lot of resources in the first place.

Text is very compressible compared to compiled binaries, thus we recommend at least 3 for local compression and 1 for remote

But obviously, each project can be different — thus it's a setting

As well as different use cases, for example when you have limited connection you might prefer compression over latency

It does eat quite a lot of resources, try 9 :)

arturdryomov · 2018-12-03T05:21:19Z

src/config.rs

+#[derive(Debug, Eq, PartialEq)]
+pub struct Compression {
+    pub local: i64,
+    pub remote: i64,


Shouldn’t it be i8 though?

yea, it's a slip of YAML impl details, I guess I should use proper type here

Not done yet, I'll track it separately, value is validated anyway and realistically i8 is going to still be i32 at least on actual hardware

arturdryomov · 2018-12-03T05:22:45Z

src/intermediate_config.rs

+
+    let compression = match &yaml["compression"] {
+        Yaml::Hash(compression) => {
+            let local = match compression.get(&Yaml::String(String::from("local"))).cloned() {


Dude, YAML parsing in Rust sucks!

well, could have been worse lol

src/intermediate_config.rs

arturdryomov · 2018-12-03T06:51:19Z

After internal discussion we decided to pursue the following.

`.mainframer/config.yml`

remote:
  name: "name"
push:
  compression: N
  sync: "serial"
pull:
  compression: N
  sync: "parallel"

`.mainframer/ignore.yml`

both:
  - "path"
  - "path"
  - "path"
push:
  - "path"
  - "path"
  - "path"
pull:
  - "path"
  - "path"
  - "path"

artem-zinnatullin · 2018-12-06T07:19:30Z

@ming13 updated PR with new YAML structure

arturdryomov · 2018-12-06T17:40:49Z

src/config.rs

-    pub local: i64,
-    pub remote: i64,
+pub struct Push {
+    pub compression: i64,


Still — i64 is too much.

Switched to u8

arturdryomov · 2018-12-06T17:41:10Z

src/config.rs

+    pub compression: i64,
+}
+
+#[derive(Debug, Eq, PartialEq)]


Why do you need both Eq and PartialEq?

That's a great question my friend!

I need equality for unit tests (assert_eq!())
In Rust Eq seems to be special case of PartialEq and most of things like assert_eq!() need PartialEq thus I definitely need to have PartialEq

Removed Eq 👍

arturdryomov · 2018-12-06T17:41:43Z

src/intermediate_config.rs

-    pub compression: Option<IntermediateCompression>,
+    pub remote: Option<IntermediateRemote>,
+    pub push: Option<IntermediatePush>,
+    pub pull: Option<IntermediatePull>,


Really weird, but I like that both push and pull have the same horizontal space...

arturdryomov · 2018-12-06T17:45:11Z

src/intermediate_config.rs

-                Some(local) => match local {
-                    Yaml::Integer(local) => if local >= 1 && local <= 9 {
-                        Some(local)
+    let push = match &yaml["push"] {


I think it is time to move push and pull reading to a single function since essentially they are the same. Maybe even replace struct Pull and struct Push with struct Sync or whatever.

They're not same, Pull will support parallel mode soon, common fields can be extracted though

arturdryomov · 2018-12-06T17:45:35Z

src/intermediate_config.rs

-    pub local: Option<i64>,
-    pub remote: Option<i64>,
+pub struct IntermediatePush {
+    pub compression: Option<i64>


compressionLevel?

Why though, it's called compression in YAML, I mean we can have different name in code but I don't see much value in that atm

…aml-config

artem-zinnatullin · 2019-01-21T07:52:41Z

Updated PR, @ming13 let's do YAML for ignore files in separate PR, it will be a pretty big change

arturdryomov · 2019-01-21T08:33:42Z

All right, I’ll try not to leave any comments since the next time you’ll have time to make changes is approximately March 26.

arturdryomov · 2019-01-21T19:35:23Z

src/config.rs

-use std::path::Path;
-
-#[derive(Debug, PartialEq, Eq)]
+#[derive(Debug, PartialEq)]


I still have no idea why language has PartialEq and Eq.

arturdryomov · 2019-01-21T19:36:05Z

Cargo.toml

@@ -9,3 +9,5 @@ version = "3.0.0-dev"
 authors = ["Artem Zinnatullin <[email protected]>", "Artur Dryomov <[email protected]>", "Mainframer Developers and Contributors"]

 [dependencies]
+yaml-rust = "0.4.2"
+linked-hash-map = "0.5.1"


Wait, what? No maps in Rust?

arturdryomov · 2019-01-21T19:38:21Z

src/intermediate_config.rs

+        };
+
+        file.read_to_string(&mut content)
+            .unwrap_or_else(|_| panic!("Could not read config file '{}'", file_path.to_string_lossy()));


arturdryomov · 2019-01-21T19:39:52Z

src/intermediate_config.rs

+    };
+
+    let pull = match &yaml["pull"] {
+        Yaml::Hash(pull) => {


Again, YAML parsing is so weird...

arturdryomov · 2019-01-21T19:40:45Z

src/intermediate_config.rs

+
+        assert_eq!(parse_config_from_str(content), Ok(IntermediateConfig {
+            remote: Some(IntermediateRemote {
+                host: Some(String::from("computer1")),


Isn’t it better to define expected variables beforehand and put them into the content?

arturdryomov · 2019-01-21T19:41:21Z

src/intermediate_config.rs

+  compression: yooo
+";
+        assert_eq!(parse_config_from_str(content), Err(String::from("'pull.compression\' must be a positive integer from 1 to 9, but was String(\n    \"yooo\"\n)")));
+    }


Pretty good test suite.

artem-zinnatullin added 3 commits November 25, 2018 01:32

Switch config to YAML

5278543

More tests

aa3b062

Remove remoteMachine.user for now

35874e8

artem-zinnatullin added the enhancement label Nov 26, 2018

artem-zinnatullin added this to the v3.0.0 milestone Nov 26, 2018

artem-zinnatullin assigned arturdryomov and artem-zinnatullin Nov 26, 2018

artem-zinnatullin requested a review from arturdryomov November 26, 2018 07:43

Add test for quotes

ea9cfaa

arturdryomov suggested changes Dec 3, 2018

View reviewed changes

artem-zinnatullin added 2 commits December 4, 2018 19:41

Change yaml structure

f6f167b

Fix test failures

ec76da7

arturdryomov suggested changes Dec 6, 2018

View reviewed changes

artem-zinnatullin mentioned this pull request Jan 11, 2019

Add reworked configuration docs. #245

Merged

artem-zinnatullin added 4 commits January 20, 2019 23:03

Use u8

3536c4f

Merge branch '3.x' of github.com:buildfoundation/mainframer into az/y…

6ca5077

…aml-config

Extract compression parsing

a761727

Remove Eq

c1393c9

arturdryomov approved these changes Jan 21, 2019

View reviewed changes

artem-zinnatullin merged commit 2cbed2d into 3.x Feb 1, 2019

artem-zinnatullin deleted the az/yaml-config branch February 1, 2019 09:16

artem-zinnatullin mentioned this pull request May 4, 2019

Use i8 for compression values #243

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Switch config to YAML #239

Switch config to YAML #239

artem-zinnatullin commented Nov 26, 2018 •

edited

Loading

arturdryomov Nov 28, 2018

artem-zinnatullin Dec 3, 2018

arturdryomov Dec 3, 2018

artem-zinnatullin Dec 3, 2018

artem-zinnatullin Dec 6, 2018

artem-zinnatullin Dec 6, 2018

arturdryomov Dec 3, 2018

artem-zinnatullin Dec 3, 2018

arturdryomov commented Dec 3, 2018

artem-zinnatullin commented Dec 6, 2018

arturdryomov Dec 6, 2018

artem-zinnatullin Jan 21, 2019

arturdryomov Dec 6, 2018

artem-zinnatullin Jan 21, 2019

arturdryomov Dec 6, 2018

arturdryomov Dec 6, 2018

artem-zinnatullin Jan 21, 2019

arturdryomov Dec 6, 2018

artem-zinnatullin Jan 21, 2019

artem-zinnatullin commented Jan 21, 2019

arturdryomov commented Jan 21, 2019

arturdryomov Jan 21, 2019

arturdryomov Jan 21, 2019

arturdryomov Jan 21, 2019

arturdryomov Jan 21, 2019

arturdryomov Jan 21, 2019

arturdryomov Jan 21, 2019

Switch config to YAML #239

Switch config to YAML #239

Conversation

artem-zinnatullin commented Nov 26, 2018 • edited Loading

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

arturdryomov commented Dec 3, 2018

.mainframer/config.yml

.mainframer/ignore.yml

artem-zinnatullin commented Dec 6, 2018

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

artem-zinnatullin commented Jan 21, 2019

arturdryomov commented Jan 21, 2019

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

artem-zinnatullin commented Nov 26, 2018 •

edited

Loading

`.mainframer/config.yml`

`.mainframer/ignore.yml`