haraka / haraka-config

Haraka config file loader and parser
https://www.npmjs.com/package/haraka-config
MIT License
10 stars 13 forks source link

add config.getInt(filename, default_value) #44

Closed msimerson closed 6 years ago

codecov[bot] commented 6 years ago

Codecov Report

Merging #44 into master will increase coverage by 0.48%. The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #44      +/-   ##
==========================================
+ Coverage   81.54%   82.02%   +0.48%     
==========================================
  Files           8        8              
  Lines         428      434       +6     
  Branches      124      126       +2     
==========================================
+ Hits          349      356       +7     
+ Misses         79       78       -1
Impacted Files Coverage Δ
readers/flat.js 82.5% <ø> (+2.5%) :arrow_up:
readers/yaml.js 100% <ø> (ø) :arrow_up:
readers/json.js 100% <ø> (ø) :arrow_up:
readers/ini.js 97.22% <ø> (ø) :arrow_up:
readers/binary.js 100% <ø> (ø) :arrow_up:
configfile.js 70.24% <ø> (ø) :arrow_up:
readers/hjson.js 100% <ø> (ø) :arrow_up:
config.js 91.11% <100%> (+0.63%) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update f501adc...8030884. Read the comment docs.

analogic commented 6 years ago

getBool and getString might be also handy

msimerson commented 6 years ago

getBool and getString might be also handy

No arguments, but some use cases should show up before we add them.

baudehlo commented 6 years ago

Not usually a fan of combined tickets like this, but I'll let this one pass as it's fairly inconsequential.

analogic commented 6 years ago

getBool and getString might be also handy

No arguments, but some use cases should show up before we add them.

I was asked to adjust parseInt(config...) at connection.js and ... well ... thats next to line config.get(smthngbool) ? true : false. I guess that there will be lot of config sanitization lines in haraka...

msimerson commented 6 years ago

Lets put some meat on that guess:

~/git/haraka $ grep config.get `gfind` | grep 'true\|false'
./plugins/rcpt_to.host_list_base.js:    const anti_spoof = plugin.config.get('host_list.anti_spoof') || false;
./connection.js:        this.banner_includes_uuid = config.get('banner_includes_uuid') ? true : false;
./connection.js:        this.header_hide_version = config.get('header_hide_version') ? true : false;

Not my definition of a lot, but there are a few. Instead of adding a getBool(), I'd rather move those connection.js configurations into smtp.ini. Ini files already have boolean support.

getString() would have a few more hits:

~/git/haraka $ grep config.get `gfind` | grep '||' | grep -v 'json\|binary\|false\|0\|1\|5\|9\|null'
./plugins/block_me.js:    const recip = (this.config.get('block_me.recipient') || '').toLowerCase();
./plugins/queue/qmail-queue.js:    plugin.queue_exec = plugin.config.get('qmail-queue.path') || '/var/qmail/bin/qmail-queue';
./outbound/config.js:        cfg.received_header = config.get('outbound.received_header') || 'Haraka outbound';
./connection.js:            host: config.get('me') || os.hostname(),
./connection.js:        this.ehlo_hello_message = config.get('ehlo_hello_message') || 'Haraka is at your service.'

As before, since connection.js is hot code, I'd rather move me and ehlo_hello_message into smtp.ini. Each connection already has access to server.cfg (the processed contents of smtp.ini) so accessing that would be faster. That doesn't leave many uses cases for getString().

analogic commented 6 years ago

You are right I've made assumptions before proper research. (and I'am scared with specialities of JS type system also)

msimerson commented 6 years ago
$ npm publish
npm notice 
npm notice 📦  haraka-config@1.0.16
npm notice === Tarball Contents === 
npm notice 965B   package.json                  
npm notice 132B   .codeclimate.yml              
npm notice 185B   .eslintrc.yaml                
npm notice 235B   .travis.yml                   
npm notice 395B   appveyor.yml                  
npm notice 1.5kB  Changes.md                    
npm notice 4.4kB  config.js                     
npm notice 13.3kB configfile.js                 
npm notice 1.1kB  LICENSE                       
npm notice 10.1kB README.md                     
npm notice 490B   run_tests                     
npm notice 421B   readers/binary.js             
npm notice 1.8kB  readers/flat.js               
npm notice 212B   readers/hjson.js              
npm notice 4.0kB  readers/ini.js                
npm notice 170B   readers/json.js               
npm notice 217B   readers/yaml.js               
npm notice 17.9kB test/config.js                
npm notice 145B   test/config/bad.yaml          
npm notice 10B    test/config/dir/1.ext         
npm notice 10B    test/config/dir/2.ext         
npm notice 10B    test/config/dir/3.ext         
npm notice 50B    test/config/goobers.ini       
npm notice 251B   test/config/main.hjson        
npm notice 67B    test/config/main.json         
npm notice 1B     test/config/me                
npm notice 24B    test/config/missing.yaml      
npm notice 38B    test/config/missing2.yaml     
npm notice 18B    test/config/override.yaml     
npm notice 29B    test/config/override2.yaml    
npm notice 120B   test/config/test.binary       
npm notice 24B    test/config/test.data         
npm notice 35B    test/config/test.flat         
npm notice 389B   test/config/test.hjson        
npm notice 519B   test/config/test.ini          
npm notice 2B     test/config/test.int          
npm notice 111B   test/config/test.json         
npm notice 24B    test/config/test.list         
npm notice 24B    test/config/test.value        
npm notice 348B   test/config/test.yaml         
npm notice 26B    test/config/tls_cert.pem      
npm notice 25B    test/config/tls_key.pem       
npm notice 15.5kB test/configfile.js            
npm notice 13B    test/default/config/test.flat 
npm notice 29B    test/default/config/test.ini  
npm notice 13B    test/override/config/test.flat
npm notice 32B    test/override/config/test.ini 
npm notice 788B   test/readers/binary.js        
npm notice 1.2kB  test/readers/flat.js          
npm notice 848B   test/readers/hjson.js         
npm notice 5.4kB  test/readers/ini.js           
npm notice 752B   test/readers/json.js          
npm notice 775B   test/readers/yaml.js          
npm notice === Tarball Details === 
npm notice name:          haraka-config                           
npm notice version:       1.0.16                                  
npm notice package size:  20.1 kB                                 
npm notice unpacked size: 85.3 kB                                 
npm notice shasum:        3dd78313649d784f25f21b302e4edaf7a1b9d8d1
npm notice integrity:     sha512-yz3OQAcZ28uIn[...]xtFR9knZELcrQ==
npm notice total files:   53                                      
npm notice 
+ haraka-config@1.0.16