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.

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

Feature/myst 27 broker on public domain #86

Merged
merged 4 commits into from Jan 8, 2018

Conversation

Projects
None yet
4 participants
@zolia
Copy link
Member

commented Jan 8, 2018

No description provided.

@@ -14,8 +14,8 @@ RUN go get github.com/debber/debber-v0.3/cmd/debber
# Compile application
WORKDIR /go/src/github.com/mysterium/node
ADD . .
RUN GOOS=linux GOARCH=amd64 bin/server_build \
&& bin/server_package_debian ${PACKAGE_VERSION} amd64
RUN GOOS=linux GOARCH=amd64 bin/client_build \

This comment has been minimized.

Copy link
@Waldz

Waldz Jan 8, 2018

Member

ups :/ 🥇

&& apt-get install -y curl \
&& curl -s https://swupdate.openvpn.net/repos/repo-public.gpg | apt-key add \
&& echo "deb http://build.openvpn.net/debian/openvpn/stable xenial main" > /etc/apt/sources.list.d/openvpn-aptrepo.list \
&& apt-get update \

This comment has been minimized.

Copy link
@Waldz

Waldz Jan 8, 2018

Member

We have 2 apt-get update statements, can we delete one?

This comment has been minimized.

Copy link
@Waldz

Waldz Jan 8, 2018

Member

Ok, but it's possible only if You download without curl

@@ -5,4 +5,6 @@ const CONTACT_NATS_V1 = "nats/v1"
type ContactNATSV1 struct {
// Topic on which client is getting message
Topic string
// NATS servers used by node and should be contacted via
BrokerAddresses []string

This comment has been minimized.

Copy link
@Waldz

Waldz Jan 8, 2018

Member
  1. Add JSON annotations
    2 Freeze JSON serialisation with tests
@@ -40,6 +40,7 @@ func TestNewAddressForContact(t *testing.T) {
Type: "nats/v1",
Definition: ContactNATSV1{
Topic: "123456",
BrokerAddresses: []string{"nats://" + natsServerIp + ":4222"},

This comment has been minimized.

Copy link
@Waldz

Waldz Jan 8, 2018

Member

Dont depend on live variables.
Just pass mocked variable like "nats://far-server:1234" and assert that it was parsed correctly

@@ -19,6 +19,13 @@ func NewAddress(server string, port int, topic string) *NatsAddress {
}
}

func NewBrokerAddresses(contact ContactNATSV1) *NatsAddress {

This comment has been minimized.

Copy link
@Waldz

Waldz Jan 8, 2018

Member

We already had such factory NewAddressForContact, lets improve same function.
And lets improve NewAddress(server, port, topic) -> NewAddress(topic, address...)

topic: contact.Topic,
}
}

func NewAddressForIdentity(identity identity.Identity) *NatsAddress {

This comment has been minimized.

Copy link
@Waldz

Waldz Jan 8, 2018

Member

Lets rename this to NewAddressGenerate() - it announces node's address

@@ -2,7 +2,7 @@ package nats_dialog

import (
"fmt"
"github.com/mgutz/logxi/v1"

This comment has been minimized.

Copy link
@donce

donce Jan 8, 2018

Contributor

Why did we have logxi in the first place?
Maybe it makes sense to remove it from `glide.lock? It will safe us in the future from using logger which is not working.

func (service *serviceIpTables) clearStaleRules() error {
return service.disableRules()
func (service *serviceIpTables) clearStaleRules() {
service.disableRules()

This comment has been minimized.

Copy link
@donce

donce Jan 8, 2018

Contributor

If deletion of forwarding rules fails, maybe it's worth at least to add some warning log? I believe that this failure of deletion can mean multiple things, and ignoring that might be dangerous.

@Waldz

Waldz approved these changes Jan 8, 2018

@zolia zolia merged commit 25c671b into master Jan 8, 2018

@zolia zolia deleted the feature/MYST-27-broker-on-public-domain branch Jan 8, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.