Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Debt: Consolidate mod processing in addRecordSRV/addRecord #157

Closed
tlimoncelli opened this issue Jul 19, 2017 · 4 comments
Closed

Debt: Consolidate mod processing in addRecordSRV/addRecord #157

tlimoncelli opened this issue Jul 19, 2017 · 4 comments

Comments

@tlimoncelli
Copy link
Contributor

There is a lot of repeated code in addRecordSRV/addRecord. It should be collapsed into a helper function.

@whs
Copy link
Contributor

whs commented Jul 27, 2017

I could take on this.

I'm thinking of something like:

var CNAME = recordBuilder({
    type: 'CNAME',
});

which should take care of the (name, target) type.

For special cases like SRV a hook could be added

var SRV = recordBuilder({
    type: 'SRV',
    args: [
        ['name', _.isString], // second argument can be omitted/null if validation is not needed
        ['priority', _.isNumber],
        ['weight', _.isNumber],
        ['port', _.isNumber],
        ['target', _.isString],
    ],
    transform: function(record, args, modifiers){
        // record is {type: 'SRV', ttl:d.defaultTTL, meta:{}}
        // with modifiers already applied
        // args will be an object for parameters defined

        record.name = args.name;
        record.srvpriority = args.priority;
        record.srvweight = args.srvweight;
        record.srvport = args.srvport;
        record.target = args.target;

        return record;
    },
    // example of number modifier parsing (eg. in CAA)
    modifierNumber: function(record, value){
        record.caaflags |= value;
        return record;
    }
});

Record could implement modifierNumber and modifierString modifierBoolean (basically all primitive types but null/undefined) to support custom record modifier like what MX/CAA are using now. The default handler for those options will log "Warning: Number/String/Boolean modifier (value) is not supported. Skipping"

Is there any use case that I missed?

@captncraig
Copy link
Contributor

captncraig commented Jul 27, 2017

I was thinking even simpler. Have addRecord return an object, and SRV just adds some fields to it after it finishes. So addRecord does the common things, runs modifiers, etc. and SRV adds its own fields after. Sometimes the over functional style is good, but others it makes things odd.

@whs
Copy link
Contributor

whs commented Jul 28, 2017

That still retain the special cases of number modifier parsing for CAA/MX. As addRecord will warn on unknown modifier, handling the number modifiers in the CAA function will also trigger the warning.

captncraig pushed a commit that referenced this issue Aug 11, 2017
* helpers.js: Refactor addRecord to recordBuilder

* Fixed failing dns test

* helpers.js: Fixed a typo in SRV handling

* helpers.js: Move type to top level argument

* Removed support for numeric modifiers

* helpers.js: Added IMPORT_TRANSFORM ttl argument back
@tlimoncelli
Copy link
Contributor Author

Fixed in #157 and #163

rblenkinsopp pushed a commit to rblenkinsopp/dnscontrol that referenced this issue Aug 21, 2020
…e#163)

* helpers.js: Refactor addRecord to recordBuilder

* Fixed failing dns test

* helpers.js: Fixed a typo in SRV handling

* helpers.js: Move type to top level argument

* Removed support for numeric modifiers

* helpers.js: Added IMPORT_TRANSFORM ttl argument back
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants