-
Notifications
You must be signed in to change notification settings - Fork 128
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.
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
acra_configui #111
acra_configui #111
Conversation
cmd/acra_configui/acra_configui.go
Outdated
|
||
// TODO: errors output, | ||
|
||
// sdfsdf |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cleaned
cmd/acra_configui/acra_configui.go
Outdated
// sdfsdf | ||
|
||
import ( | ||
//"io/ioutil" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we remove this lines? https://godoc.org/golang.org/x/tools/cmd/goimports - this tool drop unnecessary imports and add missing. it help to not care about commenting some imports. (golang IDE can run it for every .go file on save)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cleaned. Tip: IDE can manage this using "Reformat Code" action
cmd/acra_configui/acra_configui.go
Outdated
} | ||
} | ||
|
||
type paramYAML struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought how we can avoid duplicating and remembered that we can operate with interface{} or map values and convert to/from json/yaml. But when I tried use struct directly I took correct converting without hacks
package main
import (
"fmt"
"gopkg.in/yaml.v2"
"encoding/json"
)
type SomeStruct struct{
A string
B bool
C int
D []string
}
func main(){
var a SomeStruct
// var a map[string]interface{} // it works too
if err := json.Unmarshal([]byte(`{"A": "str", "B": true, "C": 123, "D": ["a1", "a2", "a3"]}`), &a); err != nil{
panic(err)
}
fmt.Println(a)
out, err := yaml.Marshal(a)
if err != nil{
panic(err)
}
fmt.Println(string(out))
}
So I think better to use one struct instead two and avoid problem with synchronization
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It fails for me in go1.5.4 with struct tags to make input/output more json/yaml convenient and compatible to current configs
cmd/acra_configui/acra_configui.go
Outdated
WithZone bool `json:"zonemode"` | ||
} | ||
|
||
type JsonResponse struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where is this struct used?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed
cmd/acra_configui/acra_configui.go
Outdated
|
||
err := r.ParseForm() | ||
if err != nil { | ||
panic(err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think better to log error and exit. Then we can collect logs and do some processing, count errors, etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree to log, but it's request processing and we should continue with return
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we can panic and log together. panic to process error as it expected by framework and add log record to process it in a future with our format, not with format of panic messages. Human message like "can't parse post form in 'getConfig' api methid" will be better than something like "incorrect url parameters format"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
log.Printf("Warning: %v\n", utils.ErrorMessage("can't send data with secure session to acraproxy", err)) | ||
response = "HTTP/1.1 500 Server error\r\n\r\n\r\n\r\n" | ||
} | ||
log.Println(configFromUI) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
log.Debugln
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
cmd/acraserver/config.go
Outdated
s.StopOnPoison = config.stopOnPoison | ||
s.WithZone = config.withZone | ||
out, err := json.Marshal(s) | ||
return string(out), err |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what about to return out as is ([]byte) and convert to string on demand on client's code? many places in golang expects []byte
and it leave ability to avoid double convert operation between string and []byte (each operation do full copy of data)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agree, done
cmd/acraserver/config.go
Outdated
return string(out), err | ||
} | ||
|
||
func (config *Config) JsonFromPOST(req *http.Request) (ConfigProxy, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think config
don't need to know anything about http.Request
and will be great to operate with []byte
on config side and operate with http.Request
on http handler side
Plus better to return reference (*ConfigProxy
) to config
instead copy of value
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
JsonFromPOST is not used any more, removed
cmd/acraserver/config.go
Outdated
@@ -41,6 +43,18 @@ type Config struct { | |||
ConnectionWrapper network.ConnectionWrapper | |||
} | |||
|
|||
type ConfigProxy struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was confused when read ConfigProxy
in acraserver
package because now acraserver shouldn't know about acraproxy and can operate with clients directly. But when read field's name I understood that you mean proxying arcaserver's config to webui config. Maybe better to rename to UIEditableConfig
or something more understandable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
accepted
cmd/acraserver/config.go
Outdated
s.ScriptOnPoison = config.scriptOnPoison | ||
s.StopOnPoison = config.stopOnPoison | ||
s.WithZone = config.withZone | ||
out, err := json.Marshal(s) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we can avoid extra struct and convert like
conf := map[string]interface{} {"ProxyHost": config.proxyHost, "ProxyPort": config.proxyPort}
out, err := json.Marshal(conf)
And better to use Getters
. Previously I said that no matter how to fetch values. But remembered that I added getter/setter for extra logic when operate with config. For example to log getting/settings, to encapsulate logic of settings if it will be more complex than just assign field
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
using getters
No description provided.