On 06/27/2017 09:08 AM, Roberto Mier Escandón  wrote:
> rereplied
> 
> Diff comments:
> 
>> diff --git a/daemon/daemon_test.go b/daemon/daemon_test.go
>> index c6242cd..a2bfca3 100644
>> --- a/daemon/daemon_test.go
>> +++ b/daemon/daemon_test.go
>> @@ -94,8 +98,76 @@ func TestManualMode(t *testing.T) {
>>   	}
>>   }
>>   
>> +type mockWifiap struct{}
>> +
>> +//var wifiapClient wifiapInterface
> 
> commented on purpose?
> 
>> +
>> +func (mock *mockWifiap) Do(req *http.Request) (*http.Response, error) {
>> +	fmt.Println("==== MY do called")
>> +	url := req.URL.String()
>> +	if url != "http://unix/v1/configuration" {
>> +		return nil, fmt.Errorf("Not valid request URL: %v", url)
>> +	}
>> +
>> +	if req.Method != "GET" {
>> +		return nil, fmt.Errorf("Method is not valid. Expected GET, got %v", req.Method)
>> +	}
>> +
>> +	rawBody := `{"result":{
>> +		"debug":false,
>> +		"dhcp.lease-time": "12h",
>> +		"dhcp.range-start": "10.0.60.2",
>> +		"dhcp.range-stop": "10.0.60.199",
>> +		"disabled": true,
>> +		"share.disabled": false,
>> +		"share-network-interface": "tun0",
>> +		"wifi-address": "10.0.60.1",
>> +		"wifi.channel": "6",
>> +		"wifi.hostapd-driver": "nl80211",
>> +		"wifi.interface": "wlan0",
>> +		"wifi.interface-mode": "direct",
>> +		"wifi.netmask": "255.255.255.0",
>> +		"wifi.operation-mode": "g",
>> +		"wifi.security": "
>> +		"wifi.security-passphrase": "passphrase123",
>> +		"wifi.ssid": "AP"},"status":"OK","status-code":200,""sync"}`
>> +
>> +	response := http.Response{
>> +		StatusCode: 200,
>> +		Status:     "200 OK",
>> +		Body:       ioutil.NopCloser(strings.NewReader(rawBody)),
>> +	}
>> +
>> +	return &response, nil
>> +}
>> +
>> +func (mock *mockWifiap) Show() (map[string]interface{}, error) {
>> +	wifiAp := make(map[string]interface{})
>> +	wifiAp["wifi.security-passphrase"] = "randompassphrase"
>> +	return wifiAp, nil
>> +}
>> +
>> +func (mock *mockWifiap) Enabled() (bool, error) {
>> +	return true, nil
>> +}
>> +
>> +func (mock *mockWifiap) Enable() error {
>> +	return nil
>> +}
>> +func (mock *mockWifiap) Disable() error {
>> +	return nil
>> +}
>> +func (mock *mockWifiap) SetSsid(s string) error {
>> +	return nil
>> +}
>> +
>> +func (mock *mockWifiap) SetPassphrase(p string) error {
>> +	return nil
>> +}
>> +
>>   func TestSetDefaults(t *testing.T) {
>>   	client := GetClient()
>> +	PreConfigFile = "../static/tests/pre-config0.json"
>>   	hfp := "/tmp/hash"
>>   	if _, err := os.Stat(hfp); err == nil {
>>   		err = os.Remove(hfp)
>> diff --git a/hooks/configure.go b/hooks/configure.go
>> new file mode 100644
>> index 0000000..c674cca
>> --- /dev/null
>> +++ b/hooks/configure.go
>> @@ -0,0 +1,111 @@
>> +// -*- Mode: Go; indent-tabs-mode: t -*-
>> +
>> +/*
>> + * Copyright (C) 2017 Canonical Ltd
>> + *
>> + * This program is free software: you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 3 as
>> + * published by the Free Software Foundation.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + * GNU General Public License for more details.
>> + *
>> + * You should have received a copy of the GNU General Public License
>> + * along with this program.  If not, see <http://www.gnu.org/licenses/>.
>> + *
>> + */
>> +
>> +package main
>> +
>> +import (
>> +	"encoding/json"
>> +	"io/ioutil"
>> +	"log"
>> +	"os/exec"
>> +	"strings"
>> +
>> +	"launchpad.net/wifi-connect/daemon"
>> +)
>> +
>> +// Client is the base struct for runtime and testing
>> +type Client struct {
>> +	getter Getter
>> +}
>> +
>> +// Get is the test obj for overridding functions
>> +type Get struct{}
>> +
>> +// Getter interface is for overriding SnapGet for testing
>> +type Getter interface {
>> +	SnapGet(string) (string, error)
>> +}
>> +
>> +// GetClient returns a normal runtime client
>> +func GetClient() *Client {
>> +	return &Client{getter: &Get{}}
>> +}
>> +
>> +// GetTestClient returns a testing client
>> +func GetTestClient(g Getter) *Client {
> 
>>From my point of view, I expect GetTestClient() to return a test client. If it can return a production client depending on received parameter, makes no sense to me having 'test' in the name.

can we agree to disagree on this or do you feel strongly?

> 
>> +	return &Client{getter: g}
>> +}
>> +
>> +// SnapGet uses snapctrl to get a value fro a key, or returns error
>> +func (g *Get) SnapGet(key string) (string, error) {
>> +	out, err := exec.Command("snapctl", "get", key).Output()
>> +	if err != nil {
>> +		return "", err
>> +	}
>> +	return strings.TrimSpace(string(out)), nil
>> +
>> +}
>> +
>> +// snapGetStr wraps SnapGet for string types and verifies the snap var is valid
>> +func (c *Client) snapGetStr(key string, target *string) {
>> +	val, err := c.getter.SnapGet(key)
>> +	if err != nil {
>> +		return
>> +	}
>> +	if len(val) > 0 {
> 
> well, It's having the only positive case at the end of the method more than saving space

I intend to do it your way :)
> 
>> +		*target = val
>> +		return
>> +	}
>> +	log.Printf("== wifi-connect/configure error: key %s exists but has zero length", key)
>> +}
>> +
>> +// snapGetBool wraps SnapGet for bool types and verifies the snap var is valid
>> +func (c *Client) snapGetBool(key string, target *bool) {
>> +	val, err := c.getter.SnapGet(key)
>> +	if err != nil {
>> +		return
>> +	}
>> +	if len(val) == 0 {
>> +		log.Printf("== wifi-connect/configure error: key %s exists but has zero length", key)
>> +		return
>> +	}
>> +
>> +	if val == "true" {
>> +		*target = true
>> +	} else {
>> +		*target = false
>> +	}
>> +}
>> +
>> +func main() {
> 
> updating snap settings after installed could break anything? (I guess not). And if not desired the config to be updated, maybe there is a way to prevent using snap set when installed? (just wondering)
> 
>> +	client := GetClient()
>> +	preConfig := &daemon.PreConfig{}
>> +	client.snapGetStr("wifi.security-passphrase", &preConfig.Passphrase)
>> +	client.snapGetStr("portal.password", &preConfig.Password)
>> +	client.snapGetBool("portal.no-operational", &preConfig.NoOperational)
>> +	client.snapGetBool("portal.no-reset-creds", &preConfig.NoResetCreds)
>> +
>> +	b, errJM := json.Marshal(preConfig)
>> +	if errJM == nil {
>> +		errWJ := ioutil.WriteFile(daemon.PreConfigFile, b, 0644)
>> +		if errWJ != nil {
>> +			log.Print("== wifi-connect/configure error:", errWJ)
>> +		}
>> +	}
>> +}
>> diff --git a/service/service.go b/service/service.go
>> index ec39f5f..07320a1 100644
>> --- a/service/service.go
>> +++ b/service/service.go
>> @@ -95,7 +100,9 @@ func main() {
>>   			if client.GetPreviousState() == daemon.MANAGING {
>>   				client.ManagementServerDown()
>>   			}
>> -			client.OperationalServerUp()
>> +			if !config.NoOperational {
> 
> iirc the reason is because you want the default values for those both params to be false. What's the problem of setting them true or false, if not provided explicitly, as default value?

That's not quite the reason I tried to explain previously. Should we 
discuss in hang out so we don't keep going round in circles on this point?

> 
>> +				client.OperationalServerUp()
>> +			}
>>   			continue
>>   		}
>>   
> 
>