Skip to content
New issue

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

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Jump to bottom

regression test for: migration: unexpected type of dns: map[string]interface #4848

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

regression test for: migration: unexpected type of dns: map[string]interface #4848

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 3 additions & 0 deletions internal/home/testdata/12to13-regression.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
schema_version: 12
Copy link
Contributor

Choose a reason for hiding this comment

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

Here, too, a TODO should be added to make it a more faithful representation of a config.yaml at version 12. With YAML, you can never be too defensive, heh.

dns:
port: 53
156 changes: 77 additions & 79 deletions internal/home/upgrade.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -240,8 +240,9 @@ func upgradeSchema3to4(diskConf yobj) error {

// Replace "auth_name", "auth_pass" string settings with an array:
// users:
// - name: "..."
// password: "..."
// - name: "..."
// password: "..."
Copy link
Contributor

Choose a reason for hiding this comment

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

Just to be clear, these format changes have been made by the Go 1.19 comment formatter as opposed to manual reformatting, right?

Copy link
Author

Choose a reason for hiding this comment

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

As sorry I missed this :-(
Yes, it is the Go 1.10 comment formatter :-/.
I can remove this if you prefer?

Copy link
Contributor

Choose a reason for hiding this comment

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

No, you don't need to, I'm just making sure.

//
// ...
func upgradeSchema4to5(diskConf yobj) error {
log.Printf("%s(): called", funcName())
Expand Down Expand Up @@ -288,16 +289,18 @@ func upgradeSchema4to5(diskConf yobj) error {

// clients:
// ...
// ip: 127.0.0.1
// mac: ...
//
// ip: 127.0.0.1
// mac: ...
//
// ->
//
// clients:
// ...
// ids:
// - 127.0.0.1
// - ...
//
// ids:
// - 127.0.0.1
// - ...
func upgradeSchema5to6(diskConf yobj) error {
log.Printf("%s(): called", funcName())

Expand Down Expand Up @@ -355,19 +358,21 @@ func upgradeSchema5to6(diskConf yobj) error {
}

// dhcp:
// enabled: false
// interface_name: vboxnet0
// gateway_ip: 192.168.56.1
// ...
//
// enabled: false
// interface_name: vboxnet0
// gateway_ip: 192.168.56.1
// ...
//
// ->
//
// dhcp:
// enabled: false
// interface_name: vboxnet0
// dhcpv4:
// gateway_ip: 192.168.56.1
// ...
//
// enabled: false
// interface_name: vboxnet0
// dhcpv4:
// gateway_ip: 192.168.56.1
// ...
func upgradeSchema6to7(diskConf yobj) error {
log.Printf("Upgrade yaml: 6 to 7")

Expand Down Expand Up @@ -443,15 +448,14 @@ func upgradeSchema6to7(diskConf yobj) error {

// upgradeSchema7to8 performs the following changes:
//
// # BEFORE:
// 'dns':
// 'bind_host': '127.0.0.1'
//
// # AFTER:
// 'dns':
// 'bind_hosts':
// - '127.0.0.1'
// # BEFORE:
// 'dns':
// 'bind_host': '127.0.0.1'
//
// # AFTER:
// 'dns':
// 'bind_hosts':
// - '127.0.0.1'
func upgradeSchema7to8(diskConf yobj) (err error) {
log.Printf("Upgrade yaml: 7 to 8")

Expand Down Expand Up @@ -481,14 +485,13 @@ func upgradeSchema7to8(diskConf yobj) (err error) {

// upgradeSchema8to9 performs the following changes:
//
// # BEFORE:
// 'dns':
// 'autohost_tld': 'lan'
//
// # AFTER:
// 'dns':
// 'local_domain_name': 'lan'
// # BEFORE:
// 'dns':
// 'autohost_tld': 'lan'
//
// # AFTER:
// 'dns':
// 'local_domain_name': 'lan'
func upgradeSchema8to9(diskConf yobj) (err error) {
log.Printf("Upgrade yaml: 8 to 9")

Expand Down Expand Up @@ -564,16 +567,15 @@ func addQUICPort(ups string, port int) (withPort string) {

// upgradeSchema9to10 performs the following changes:
//
// # BEFORE:
// 'dns':
// 'upstream_dns':
// - 'quic://some-upstream.com'
//
// # AFTER:
// 'dns':
// 'upstream_dns':
// - 'quic://some-upstream.com:784'
// # BEFORE:
// 'dns':
// 'upstream_dns':
// - 'quic://some-upstream.com'
//
// # AFTER:
// 'dns':
// 'upstream_dns':
// - 'quic://some-upstream.com:784'
func upgradeSchema9to10(diskConf yobj) (err error) {
log.Printf("Upgrade yaml: 9 to 10")

Expand Down Expand Up @@ -623,15 +625,14 @@ func upgradeSchema9to10(diskConf yobj) (err error) {

// upgradeSchema10to11 performs the following changes:
//
// # BEFORE:
// 'rlimit_nofile': 42
//
// # AFTER:
// 'os':
// 'group': ''
// 'rlimit_nofile': 42
// 'user': ''
// # BEFORE:
// 'rlimit_nofile': 42
//
// # AFTER:
// 'os':
// 'group': ''
// 'rlimit_nofile': 42
// 'user': ''
func upgradeSchema10to11(diskConf yobj) (err error) {
log.Printf("Upgrade yaml: 10 to 11")

Expand All @@ -658,12 +659,11 @@ func upgradeSchema10to11(diskConf yobj) (err error) {

// upgradeSchema11to12 performs the following changes:
//
// # BEFORE:
// 'querylog_interval': 90
//
// # AFTER:
// 'querylog_interval': '2160h'
// # BEFORE:
// 'querylog_interval': 90
//
// # AFTER:
// 'querylog_interval': '2160h'
func upgradeSchema11to12(diskConf yobj) (err error) {
log.Printf("Upgrade yaml: 11 to 12")
diskConf["schema_version"] = 12
Expand Down Expand Up @@ -698,16 +698,15 @@ func upgradeSchema11to12(diskConf yobj) (err error) {

// upgradeSchema12to13 performs the following changes:
//
// # BEFORE:
// 'dns':
// # …
// 'local_domain_name': 'lan'
//
// # AFTER:
// 'dhcp':
// # …
// 'local_domain_name': 'lan'
// # BEFORE:
// 'dns':
// # …
// 'local_domain_name': 'lan'
//
// # AFTER:
// 'dhcp':
// # …
// 'local_domain_name': 'lan'
func upgradeSchema12to13(diskConf yobj) (err error) {
log.Printf("Upgrade yaml: 12 to 13")
diskConf["schema_version"] = 13
Expand All @@ -720,7 +719,7 @@ func upgradeSchema12to13(diskConf yobj) (err error) {
var dns yobj
dns, ok = dnsVal.(yobj)
if !ok {
return fmt.Errorf("unexpected type of dns: %T", dnsVal)
return fmt.Errorf("unexpected type of dns: %T; want: %T", dnsVal, dns)
Copy link
Contributor

Choose a reason for hiding this comment

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

There are a few other similar errors throughout the recent migrations, and it would be nice to have them all in a unified format.

Copy link
Author

Choose a reason for hiding this comment

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

I had added this to help me understand better what the code expected, but I agree it is more a debug printf.

I am not sure I understand your comment thou: do you want me to revert this change or to extend it to all the similar "unexpected ..." ?

Copy link
Contributor

Choose a reason for hiding this comment

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

I mean that it's better to replace all instances of errors like unexpected type of %s: %T with this new form that includes the expected type as opposed to just here.

}

dhcpVal, ok := diskConf["dhcp"]
Expand All @@ -744,23 +743,22 @@ func upgradeSchema12to13(diskConf yobj) (err error) {

// upgradeSchema13to14 performs the following changes:
//
// # BEFORE:
// 'clients':
// - 'name': 'client-name'
// # …
//
// # AFTER:
// 'clients':
// 'persistent':
// - 'name': 'client-name'
// # …
// 'runtime_sources':
// 'whois': true
// 'arp': true
// 'rdns': true
// 'dhcp': true
// 'hosts': true
// # BEFORE:
// 'clients':
// - 'name': 'client-name'
// # …
//
// # AFTER:
// 'clients':
// 'persistent':
// - 'name': 'client-name'
// # …
// 'runtime_sources':
// 'whois': true
// 'arp': true
// 'rdns': true
// 'dhcp': true
// 'hosts': true
func upgradeSchema13to14(diskConf yobj) (err error) {
log.Printf("Upgrade yaml: 13 to 14")
diskConf["schema_version"] = 14
Expand Down
14 changes: 14 additions & 0 deletions internal/home/upgrade_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,13 +1,15 @@
package home

import (
"os"
"testing"
"time"

"github.com/AdguardTeam/golibs/testutil"
"github.com/AdguardTeam/golibs/timeutil"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
"gopkg.in/yaml.v3"
)

// TODO(a.garipov): Cover all migrations, use a testdata/ dir.
Expand Down Expand Up @@ -558,6 +560,18 @@ func TestUpgradeSchema12to13(t *testing.T) {
}
}

func TestUpgradeSchema12to13Regression(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel like a better regression test would try to migrate a configuration file going all the way back to version 1. Please add a TODO assigned to me about that.

body, err := os.ReadFile("testdata/12to13-regression.yaml")
assert.NoError(t, err)
Copy link
Contributor

Choose a reason for hiding this comment

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

This one should be a require, and also split from the rest by an empty line.

diskConf := yobj{}
err = yaml.Unmarshal(body, &diskConf)
assert.NoError(t, err)

err = upgradeSchema12to13(diskConf)

assert.NoError(t, err)
Copy link
Contributor

Choose a reason for hiding this comment

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

We usually group asserts and requires together with the statements where the data was acquired, so please remove the empty line above.

}

func TestUpgradeSchema13to14(t *testing.T) {
const newSchemaVer = 14

Expand Down