Skip to content

core: Refactor to use better cli command framework#177

Merged
captncraig merged 34 commits intomasterfrom
commands
Sep 13, 2017
Merged

core: Refactor to use better cli command framework#177
captncraig merged 34 commits intomasterfrom
commands

Conversation

@captncraig
Copy link
Copy Markdown
Contributor

This breaks up the command switching logic in main into explicit commands that can be run independently.

Uses https://github.com/urfave/cli as its cli library.

@tlimoncelli
Copy link
Copy Markdown
Contributor

New requirement: This change can't break any TeamCity builds. The existing commands must work, or TC must be reconfigured to use commands that work in the old and new flags.

Currently the commands used by TeamCity are:

go test -v -verbose -provider BIND
bin/dnscontrol-Linux -providers bind -bindtree "$MDIR" push
bin\dnscontrol.exe push

Copy link
Copy Markdown
Contributor

@tlimoncelli tlimoncelli left a comment

Choose a reason for hiding this comment

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

Generally looks good. My suggestion are mostly textual.

)

var debugPreprocessCommand = &cli.Command{
Name: "debug-preprocess",
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggestion: s/debug-preprocess/output-ir/g

cmd/debugJs.go Outdated

var debugJSCommand = &cli.Command{
Name: "debug-js",
Usage: "Execute javascript and print generated json",
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

s/Execute javascript and print generated json/Output intermediate representation (IR) after JavaScript is executed but before any validation/normalization/g

cmd/debugJs.go Outdated
}

var debugJSCommand = &cli.Command{
Name: "debug-js",
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

s/debug-js/debug-otto/g

Copy link
Copy Markdown
Contributor Author

@captncraig captncraig Aug 28, 2017

Choose a reason for hiding this comment

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

I'd rather not mention otto in the command. There are other js interpreters I may evaluate and try to plug in in the future. otto is just one implementation.


var debugPreprocessCommand = &cli.Command{
Name: "debug-preprocess",
Usage: "Run validation and normalization logic, and print resulting json",
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Output intermediate representation (IR) after running validation and normalization logic.

return strings.Split(string(out), "\n"), nil
}

func checkDirectOutput() error {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Missing comment.

cmd/commands.go Outdated
cli.StringFlag{
Name: "out",
Destination: &args.Output,
Usage: "File to write json to",
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Set output file (default stdout)

cmd/commands.go Outdated
cli.StringFlag{
Name: "domains",
Destination: &args.Domains,
Usage: `Comma seperated list of domain names to include`,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

spell check: separated

}
}

func (args *FilterArgs) shouldRunProvider(p string, dc *models.DomainConfig, nonDefaultProviders []string) bool {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Missing comment

return false
}

func (args *FilterArgs) shouldRunDomain(d string) bool {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Missing comment

return nil
}

// InitializeProviders takes a creds file path and a DNSConfig object. Creates all providers with the proper types, and returns them.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Comments should be limited to 80 cols.

GetDNSConfigArgs
GetCredentialsArgs
FilterArgs
Delay int
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

PowerShell flags are missing: -fakeps, -psout, -pslog

@tlimoncelli
Copy link
Copy Markdown
Contributor

We need to add a "dnscontrol check" which is like preview, but never talks to the providers. IT needs a way to check their work without having creds. A simple syntax check (plus normalize + validate) is sufficient. With the old system running "dnscontrol check" did this and we need to retain that.

@captncraig
Copy link
Copy Markdown
Contributor Author

Pretty sure that check case is covered by output-ir

@captncraig captncraig merged commit 1d9d2b1 into master Sep 13, 2017
@captncraig captncraig changed the title Refactor to use better cli command framework core: Refactor to use better cli command framework Sep 15, 2017
@tlimoncelli tlimoncelli deleted the commands branch September 26, 2017 18:54
rblenkinsopp pushed a commit to rblenkinsopp/dnscontrol that referenced this pull request Aug 21, 2020
* starting to refactor commands

* work

* not sure

* all commands working!

* actually add file

* work in delay flag again

* start to refactor out console printing

* i hate line endings

* simple travis test to find direct output

* remove all direct printing from push/preview

* checkin vendor

* don't need this yet

* forgot to commit these

* make version explicit command

* some code review

* Add "check" subcommand.

* move stuff to commands package

* fix

* comment out check for printlns. for now

* alphabet hax

* activedir flags gone. use creds instead

* active dir doc update

* remove bind specific flags. creds instead

* default to zones dir

* fix linux build

* fix test

* cleanup random global* vars

* Clean up PowerShell docs

* rename dump-ir to print-ir. combine with print-js
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants