Conversation
…nagement # Conflicts: # management/proto/management.pb.go # management/proto/management.proto # management/server/management_test.go
| if peer.AccountId == accountId { | ||
| peers = append(peers, peer) | ||
| } | ||
| } |
There was a problem hiding this comment.
Why did you move peers outside of account? It's ok for now but previous version was better
There was a problem hiding this comment.
I did it to make it a bit more explicit and clearly separate the Account entity from Peer in the store (so that they are handled separately).
But I agree it looked better before, so I've changed it back and removed a couple of store methods
|
|
||
| account, accountFound := s.Accounts[accountId] | ||
| if !accountFound { | ||
| return nil, status.Errorf(codes.Internal, "account not found") |
There was a problem hiding this comment.
With Go, this function doesn't make a lot of sense because every call to it is exact function implementation anyway. Only error message is standard
account, err := s.GetAccount(accountId)
if err != nil {
return nil, err
}
return account, nil
There was a problem hiding this comment.
Fixed with return s.GetAccount(accountId)
| func init() { | ||
| mgmtCmd.Flags().IntVar(&mgmtPort, "port", 33073, "server port to listen on") | ||
| mgmtCmd.Flags().StringVar(&mgmtDataDir, "datadir", "/var/lib/wiretrustee/", "server data directory location") | ||
| mgmtCmd.Flags().StringVar(&mgmtHostsConfig, "hosts-config", "/etc/wiretrustee/hosts-config.json", "Wiretrustee system hosts config (STUN, TURN, Signal, etc). These will be advertised to peers ") |
There was a problem hiding this comment.
I wouldn't go with a config file only for hosts. A single config file with all the parameters makes more sense and follows more the standard of services we see, like MySQL, Apache, and Nginx.
The tricky part is to balance what you have in the config file and the flags you are providing at runtime, which should take precedence over the configuration from the file. e.g., port and letsencrypt-domain.
For hosts configuration, I understand the issue is the way we are providing them, but the way it is presented in the JSON file is not friendly and we could use the URL scheme to indicate TCP and UDP protocols. This would allow us to keep the scheme of the CLI when running the init command.
An improvement for making it more flexible to run in containers is to support configs provided by environment variables.
* feature: add config properties to the SyncResponse of the management gRpc service * fix: lint errors * chore: modify management protocol according to the review notes * fix: management proto fields sequence * feature: add proper peer configuration to be synced * chore: minor changes * feature: finalize peer config management * fix: lint errors * feature: add management server config file * refactor: extract hosts-config to a separate file * refactor: review notes applied to correct file_store usage * refactor: extract management service configuration to a file * refactor: simplify management config
3 participants
No description provided.