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

feat(commands/run): introduce --client-version and --client-port #194

Merged
merged 3 commits into from Sep 13, 2018

Conversation

Projects
None yet
2 participants
@PascalPrecht
Copy link
Contributor

PascalPrecht commented Sep 10, 2018

The important commit here is the third one. It might make sense to squash them all into a single one, in case you want to preserve the refactoring in the commit history.

},
return clone('https://github.com/aragon/aragon', CLIENT_PATH, {
checkout: clientVersion
})

This comment has been minimized.

@PascalPrecht

PascalPrecht Sep 10, 2018

Contributor

If the checkout fails because an invalid version is passed down to this command, CLI will fail with

'git checkout' failed with status code 1

I thought it'd be nicer to throw a more descriptive error message, letting the user know that the given version probably doesn't exist, however, hooking into clone()'s callback to rethrow the error, results in TaskList actually finishing its tasks successfully, with the error being throw slightly later (as it's async).

This is very likely related to how Listr expects errors to be thrown but needs more investigation.

@@ -85,6 +86,9 @@ exports.builder = function (yargs) {
description: 'Arguments for calling the app init function',
array: true,
default: [],
}).option('client-version', {

This comment has been minimized.

@PascalPrecht

PascalPrecht Sep 10, 2018

Contributor

--client-app-version ?

This comment has been minimized.

@izqui

izqui Sep 11, 2018

Member

I like --client-version more

@@ -85,6 +86,9 @@ exports.builder = function (yargs) {
description: 'Arguments for calling the app init function',
array: true,
default: [],
}).option('client-version', {
description: 'Version of Aragon client used to run your sandboxed app',
default: null

This comment has been minimized.

@izqui

izqui Sep 11, 2018

Member

I think we should make defaultClientVersion the default value for the flag so the version of the client that will be used by default is exposed to users doing aragon run --help.

This comment has been minimized.

@PascalPrecht

PascalPrecht Sep 11, 2018

Contributor

Yes, very good idea.

@@ -32,9 +33,9 @@ const { Writable } = require('stream')
const url = require('url')

const TX_MIN_GAS = 10e6
const WRAPPER_PORT = 3000
const CLIENT_PORT = 3000

This comment has been minimized.

@izqui

izqui Sep 11, 2018

Member

I'm thinking that we could use this opportunity to make the client port configurable via a flag as well.

This comment has been minimized.

@PascalPrecht

PascalPrecht Sep 11, 2018

Contributor

As discussed offline, it turns out CLIENT_PORT is not actually used when the aragon client is started.

More info: #198

This comment has been minimized.

@PascalPrecht

PascalPrecht Sep 11, 2018

Contributor

I propose fixing that in a separate PR so that this one isn't dependent on coordinating updates with CLI and aragon/aragon.

@@ -85,6 +86,9 @@ exports.builder = function (yargs) {
description: 'Arguments for calling the app init function',
array: true,
default: [],
}).option('client-version', {

This comment has been minimized.

@izqui

izqui Sep 11, 2018

Member

I like --client-version more

@PascalPrecht PascalPrecht force-pushed the PascalPrecht:feat/run/configurable-aragon-version branch from 3255b37 to e9776c0 Sep 11, 2018

@PascalPrecht

This comment has been minimized.

Copy link
Contributor

PascalPrecht commented Sep 11, 2018

@izqui I've updated this PR to use the default client version as default value.

I also went ahead and moved the client port out of the code base to make it configurable via package.json and a newly introduced --client-port option.

Notice however that, as mentioned in #194 (comment), this options is being ignored (and has always been).

I'll address that in an additional PR.

},
"aragon": {
"clientVersion": "fa2025232330bc3e4d3b792cab4bf44d754d33e6",
"clientPort": "7000"

This comment has been minimized.

@izqui

izqui Sep 11, 2018

Member

Just a reminder so we don't forget reverting the default port back to 3000

@PascalPrecht PascalPrecht force-pushed the PascalPrecht:feat/run/configurable-aragon-version branch from e9776c0 to 2ed542c Sep 11, 2018

PascalPrecht added some commits Sep 10, 2018

refactor: move aragon client version into package.json property
NPM's package files support custom properties for library specific usages.
This commit moves the aragon client version into a newly introduced
`aragon` properties section.
feat(commands/run): introduce --client-version and --client-port
This commit introduces a new option `--client-version` that can be used to
specify the Aragon client version to be used as discussed in #182. A version
can be any commit-ish values (hashes, branches, tags).

Example:

```
$ aragon run --client-version=fa2025232330bc3e4d3b792cab4bf44d754d33e6
```
Or

```
$ aragon run --client-version=dev
```

It also offers a new option `--client-port` to configure the port on which
the client is being served from.

Notice that this option is being ignored at the moment, which will be addressed
in a future commit. For more info: #198

Closes #182

@PascalPrecht PascalPrecht force-pushed the PascalPrecht:feat/run/configurable-aragon-version branch from 2ed542c to 2fcd18b Sep 11, 2018

@PascalPrecht

This comment has been minimized.

Copy link
Contributor

PascalPrecht commented Sep 11, 2018

@izqui updated.

@izqui

izqui approved these changes Sep 12, 2018

@izqui izqui changed the title feat(commands/run): introduce --client-version feat(commands/run): introduce --client-version and --client-port Sep 13, 2018

@izqui izqui merged commit 2eaea12 into aragon:master Sep 13, 2018

1 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
license/cla Contributor License Agreement is signed.
Details

@PascalPrecht PascalPrecht deleted the PascalPrecht:feat/run/configurable-aragon-version branch Sep 13, 2018

galactusss added a commit to galactusss/aragon-cli that referenced this pull request Jan 5, 2019

feat(commands/run): introduce --client-version and --client-port (ara…
…gon#194)

* refactor: move aragon client version into package.json property

NPM's package files support custom properties for library specific usages.
This commit moves the aragon client version into a newly introduced
`aragon` properties section.

* refactor: rename WRAPPER_PATH, WRAPPER_PORT to CLIENT_PATH, CLIENT_PORT

* feat(commands/run): introduce --client-version and --client-port

This commit introduces a new option `--client-version` that can be used to
specify the Aragon client version to be used as discussed in aragon#182. A version
can be any commit-ish values (hashes, branches, tags).

Example:

```
$ aragon run --client-version=fa2025232330bc3e4d3b792cab4bf44d754d33e6
```
Or

```
$ aragon run --client-version=dev
```

It also offers a new option `--client-port` to configure the port on which
the client is being served from.

Notice that this option is being ignored at the moment, which will be addressed
in a future commit. For more info: aragon#198

Closes aragon#182
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment