Feature/save dns records to hosts #172
Conversation
Corrected a few more references to rig, and elaborated the build script.
Release 1.2.0
1.2.1 release
1.2.2 Release
1.2.3 Release
Version 1.3.0
Version 1.3.1
1.3.2 Release
2.0.0 release
2.1.1 release
* Add explicit privilege prompt to improve sudo UX (phase2#138) * Explicitly prompt for privilege escallation * Remove password prompt part of privilege message * Expand sudo detection. * Tidy up timing issues. * Consolidate messaging and avoid newline in verbose. * Cleanup ToString, sudo contains, cover more exec methods. * Lint does not catch all of fmt. * Remove unnecessary password prompt from networking cleanup. * Remove color reset and cat /dev/null to clear route text. * trying a different approach to requesting for admin privs (phase2#144)
Release 2.1.3
|
On Windows the file is |
| } | ||
|
|
||
| printDNSRecords(records) |
There was a problem hiding this comment.
if you do a save or remove you likely don't want the records printed out too so maybe we only print the records if save or remove have not been specified
There was a problem hiding this comment.
They always return if they do a save or remove. So it just falls through to the default if they don't specify either.
| newHostEntries := stripDNS(oldHostEntries) | ||
| // records.String does the formatting, so convert both to a string | ||
| oldHosts := strings.Join(oldHostEntries, "\n") | ||
| newHosts := strings.Join(newHostEntries, "\n") + "\n" + |
There was a problem hiding this comment.
You could also do something like this...
fmt.Sprintf(`%s
%s
%s`, x, y, z)
to get the newlines you want.
|
The other thing that comes to mind here is how many people may get hung up because they have entries in their hosts files that they haven't removed. Maybe add a doctor check for DNS entries in |
|
I can add a check that the rows are there. It's easy enough to look for, since I'm using the pre/postamble. If you do a --save and the rows are already they, they get removed and recreated. |
| type DNSRecordsList []*DNSRecord | ||
|
|
||
| const ( | ||
| unixHostsPreamble = "##+++ added by rig" |
There was a problem hiding this comment.
I think you'll use this regardless of the underlying os as markers for starting/removing items so perhaps just call this hostsPreamble
There was a problem hiding this comment.
Good point. Thanks.
| for id, value := range dnsdockMap { | ||
| record := value.(map[string]interface{}) | ||
| record["Id"] = id | ||
| records := make([]*DNSRecord, 0, 20) |
There was a problem hiding this comment.
Interested in your thoughts behind the use of the capacity argument here. As near as I can tell this doesn't set any upper bound on the size of the array though I had to write a quick test to make sure. Is the initial capacity you picked arbitrary (both here and below where you used 10) or is there something behind that particular value?
There was a problem hiding this comment.
Completely arbitrary. Yes, when accumulating the array contents via append() more space will be allocated if needed.
There was a problem hiding this comment.
Given that it's a dynamic array perhaps just drop the capacity argument here and anywhere it doesn't have an obvious purpose. This is to prevent confusing anyone in the future about why it is there, how it got picked, does it actually result in enforced bounds, etc? Alternatively, comment something to the effect of what I think may be true about it:
// capacity picked arbitrarily with a size likely to prevent need to reallocate underlying arrays while appending items in most cases
I also presume that has some potential performance benefit though at the scales here I think it's unobservable.
If that comment isn't accurate I'd be interested in what the benefits are of declaring a capacity are in the case where append is going to be used. When using copy it's an obvious useful item to restrict the number of items you want from a potentially larger array.
Thanks for the pointers as I drag myself deeper into go.
|
Can this be tagged "In Progress". Adding Windows support. |
|
This has a bunch of merges in it. Do you need to rebase on |
Unix only so far. Need to do some research to support Windows (and someone to test).