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

MYST-549 Terms & conditions #262

Merged
merged 9 commits into from Jul 5, 2018

Conversation

Projects
None yet
5 participants
@donce
Copy link
Contributor

commented Jun 8, 2018

  • terms flag for node
  • summary terms text
  • link to TERMS.md on github
  • agreed-terms-and-conditions flag by default on docker_compose scripts

@donce donce requested review from tadovas, interro, Waldz and zolia Jun 8, 2018

@@ -11,5 +11,6 @@ exec /usr/bin/mysterium_server \
--config-dir=$OS_DIR_CONFIG \
--data-dir=$OS_DIR_DATA \
--runtime-dir=$OS_DIR_RUN \
-agreed-terms-and-conditions=$AGREED_TERMS_AND_CONDITIONS \

This comment has been minimized.

Copy link
@Waldz

Waldz Jun 12, 2018

Member

We just dropped environmental variables, lets not introduce them again

This comment has been minimized.

Copy link
@donce

donce Jun 12, 2018

Author Contributor

Do you have some alternatives, which would work with Docker and Docker-Compose?

This comment has been minimized.

Copy link
@Waldz

Waldz Jun 12, 2018

Member

We migrated to args style see e2e/docker-compose.yml

This comment has been minimized.

Copy link
@donce

donce Jun 12, 2018

Author Contributor

If I understand correctly, declaring arg values in e2e/docker-compose.yml is fine, but we need user input for terms & conditions - this approach does not work for this.

Is this what you meant? Do you have other suggestions?

This comment has been minimized.

Copy link
@Waldz

Waldz Jun 13, 2018

Member

It's user decision how to given input.
docker run mysteriumnetwork/mysterium-node:latest --agreed-terms-and-conditions=...

@ignasbernotas

This comment has been minimized.

Copy link
Member

commented Jun 13, 2018

Aren't we supposed to store the terms and conditions once they accept like in Mysterion? Need to check up with #legal

@donce donce referenced this pull request Jun 13, 2018

Closed

Remove docker-compose.yml #266

@@ -25,6 +25,8 @@ services:
--discovery-address=http://discovery/v1
--location.country=e2e-land
--openvpn.port=3000
environment:

This comment has been minimized.

Copy link
@Waldz

Waldz Jun 14, 2018

Member

command: --agreed-terms-and-conditions=04-06-2018

@donce donce closed this Jun 21, 2018

@Waldz Waldz reopened this Jul 3, 2018

@zolia zolia force-pushed the feature/MYST-549-terms-conditions branch from 1960daf to cd7b76f Jul 5, 2018

donce and others added some commits Jun 7, 2018

@zolia zolia force-pushed the feature/MYST-549-terms-conditions branch from cd7b76f to 17b60d4 Jul 5, 2018

## Running multiple nodes and clients at once

```bash
To run small network of nodes and clients in docker, you can use:

This comment has been minimized.

Copy link
@Waldz

Waldz Jul 5, 2018

Member

Localnet instruction is outdated, it's not working anymore

INSTALL.md Outdated

### Running
```bash
sudo docker start mysterium-node
sudo docker stop mysterium-node
```

Note: to run server, you will have to accept terms & conditions.
For more info about that, please read *Accepting terms & conditions* in [README.md](./README.md).

This comment has been minimized.

Copy link
@Waldz

Waldz Jul 5, 2018

Member

File INSTALL is distributed together with executables, it can not contain links to non existing README

README.md Outdated
```bash
sudo apt-get install docker-ce
sudo docker run --cap-add NET_ADMIN --net host --publish "1194:1194" --name mysterium-node -d mysteriumnetwork/mysterium-node:latest
sudo docker run --cap-add NET_ADMIN --net host --publish "1194:1194" --name mysterium-node -d mysteriumnetwork/mysterium-node:latest -agreed-terms-and-conditions

This comment has been minimized.

Copy link
@Waldz

Waldz Jul 5, 2018

Member

Put same instruction to DockerHub page

This comment has been minimized.

Copy link
@zolia

zolia Jul 5, 2018

Member

updated

@@ -4,4 +4,5 @@ sudo ./build/server/mysterium_server \
--config-dir=build/server/config \
--runtime-dir=build/server \
--localnet \
-agreed-terms-and-conditions \

This comment has been minimized.

Copy link
@Waldz

Waldz Jul 5, 2018

Member

- -> --

@@ -13,7 +13,7 @@ RuntimeDirectory=mysterium-node
RuntimeDirectoryMode=0750

EnvironmentFile=-/etc/default/mysterium-node
ExecStart=/usr/bin/mysterium_server $CONF_DIR $RUN_DIR $DISCOVERY $BROKER $PROTO
ExecStart=/usr/bin/mysterium_server -agreed-terms-and-conditions $CONF_DIR $RUN_DIR $DISCOVERY $BROKER $PROTO

This comment has been minimized.

Copy link
@Waldz

Waldz Jul 5, 2018

Member

Can we cave --agreed-terms at the end

flags.BoolVar(
&options.Version,
"version",
false,
"Show version",
)

This comment has been minimized.

Copy link
@Waldz

Waldz Jul 5, 2018

Member

Why to break the group?


func startWithOptions(options server.CommandOptions) {
if !options.AgreedTermsConditions {
fmt.Print(server.TermsNotice + "\n")

This comment has been minimized.

Copy link
@Waldz

Waldz Jul 5, 2018

Member

\n -> fmt.Println() because it's cross-platform friendly

@@ -0,0 +1,587 @@
/*

This comment has been minimized.

Copy link
@Waldz

Waldz Jul 5, 2018

Member

Not needed anymore, because it's metadata/license.go

"testing"
)

func TestGetStartupLicense(t *testing.T) {

This comment has been minimized.

Copy link
@Waldz

Waldz Jul 5, 2018

Member

Same here

`

// TermsNotice returns terms & conditions with explanation on how to agree with them
var TermsNotice = termsAndConditions + "\n\n" + explanationString

This comment has been minimized.

Copy link
@Waldz

Waldz Jul 5, 2018

Member

Would be nice to have this in package metadata with same style as metadata/license.go

@zolia zolia dismissed their stale review via 703ee8e Jul 5, 2018

@zolia zolia force-pushed the feature/MYST-549-terms-conditions branch from 17b60d4 to 703ee8e Jul 5, 2018

@zolia zolia force-pushed the feature/MYST-549-terms-conditions branch from 703ee8e to 65bed85 Jul 5, 2018

@@ -37,13 +37,16 @@ docker-compose up broker
# Start node
bin/server_build
bin/server_run
bin/server_run -agreed-terms-and-conditions

This comment has been minimized.

Copy link
@Waldz

Waldz Jul 5, 2018

Member

Suggest to remove this

# Client connects to node
bin/client_build
bin/client_run
```

Note: to run server, you will have to accept terms & conditions.

This comment has been minimized.

Copy link
@Waldz

Waldz Jul 5, 2018

Member

Suggest to remove this, cause contributors dont need terms at all

@@ -17,6 +17,7 @@ services:
- NET_ADMIN
ports:
- 11941:1194
command: --agreed-terms-and-conditions

This comment has been minimized.

Copy link
@Waldz

Waldz Jul 5, 2018

Member

You forgot to update localnet/docker-compose.yml

This comment has been minimized.

Copy link
@zolia

zolia Jul 5, 2018

Member

localnet does not starts nodes

@tadovas

tadovas approved these changes Jul 5, 2018

Copy link
Member

left a comment

LGTM

@Waldz

Waldz approved these changes Jul 5, 2018

@zolia zolia merged commit bc33bc7 into master Jul 5, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@zolia zolia deleted the feature/MYST-549-terms-conditions branch Jul 5, 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.