Skip to content
This repository has been archived by the owner on Aug 2, 2021. It is now read-only.

cmd/swarm: make bzzaccount flag optional and add bzzkeyhex flag #1531

Merged
merged 12 commits into from
Jul 4, 2019
Merged

cmd/swarm: make bzzaccount flag optional and add bzzkeyhex flag #1531

merged 12 commits into from
Jul 4, 2019

Conversation

skylenet
Copy link
Contributor

@skylenet skylenet commented Jul 2, 2019 •
edited
Loading

Flag: --bzzaccount

This flag should not be mandatory.

  • If there is no account, it will create one.
  • If a single account exists, it will try to use it.
  • If multiple accounts exist, it will notify the user about them.

I've also removed the entrypoint script from our docker image which was mainly doing the account creation.

Relates to: #1100

Flag: --bzzkeyhex

To be used for testing purposes, similar to the existing --nodekeyhex.

Specially useful on simulations, for reproducibility.

@skylenet skylenet requested review from nonsense and acud July 2, 2019 13:51
@skylenet
Copy link
Contributor Author

skylenet commented Jul 2, 2019

As noticed by @nonsense : One thing that we should probably also do is to use a different default data directory.
Currently it's using ~/.ethereum and we should probably change it to ~/.swarm

@skylenet
Copy link
Contributor Author

skylenet commented Jul 2, 2019 •
edited
Loading

There was a concern raised that picking up the first account when multiple accounts exists, could be a bad idea. To solve this we could print the list of accounts and tell the user to rerun swarm with the -bzzaddr flag?

Update: When multiple accounts exist, it will list all accounts and exit.

nonsense
nonsense reviewed Jul 2, 2019
View reviewed changes
cmd/swarm/main.go Outdated Show resolved Hide resolved
@skylenet skylenet changed the title swarm/cmd: don't require bzzaccount swarm/cmd: don't require bzzaccount flag Jul 3, 2019
@skylenet skylenet marked this pull request as ready for review July 3, 2019 15:02
@nonsense nonsense requested a review from cobordism July 3, 2019 15:02
@nonsense
Copy link
Contributor

nonsense commented Jul 3, 2019

@homotopycolimit adding you here for awareness as I know you care about those flags and the UX in general.

nonsense
nonsense reviewed Jul 3, 2019
View reviewed changes
cmd/swarm/config.go Outdated Show resolved Hide resolved
nonsense
nonsense reviewed Jul 3, 2019
View reviewed changes
cmd/swarm/config_test.go Outdated Show resolved Hide resolved
@cobordism
Copy link

@homotopycolimit adding you here for awareness as I know you care about those flags and the UX in general.

yes thanks. this has been a pet peeve of mine for a long time.

In a related note, what do you think should happen when you type swarm as a command instead of swarm up or swarm manifest or swarm version? Should swarm be started with swarm start and should swarm on its own just print some usage information, or do you want to keep current functionality?

nonsense
nonsense reviewed Jul 3, 2019
View reviewed changes
cmd/swarm/main.go Outdated Show resolved Hide resolved
nonsense
nonsense reviewed Jul 3, 2019
View reviewed changes
cmd/swarm/main.go Outdated Show resolved Hide resolved
@nonsense
Copy link
Contributor

nonsense commented Jul 3, 2019 •
edited
Loading

@homotopycolimit adding you here for awareness as I know you care about those flags and the UX in general.

yes thanks. this has been a pet peeve of mine for a long time.

In a related note, what do you think should happen when you type swarm as a command instead of swarm up or swarm manifest or swarm version? Should swarm be started with swarm start and should swarm on its own just print some usage information, or do you want to keep current functionality?

I think swarm should just print usage, and swarm daemon or swarm start should probably start the node.

I also think the default swarm node should be light node - only supporting retrieve requests and uploads, and not acting as a sync/regular node by default. sync/regular nodes should be a harder barrier to entry - you shouldn't be able to just spin up 1000 of them easily.

@cobordism
Copy link

ok.
Shall we change it so that swarm just prints info and swarm TBD starts a full node (and change the default to light node once we have light nodes?).
I think that is just a small change in code,
a smallish change in cluster configs,
and a somewhat larger change to the docs ;)

@skylenet skylenet changed the title swarm/cmd: don't require bzzaccount flag swarm/cmd: make bzzaccount flag optional and add bzzkeyhex flag Jul 4, 2019
@skylenet skylenet changed the title swarm/cmd: make bzzaccount flag optional and add bzzkeyhex flag cmd/swarm: make bzzaccount flag optional and add bzzkeyhex flag Jul 4, 2019
@skylenet skylenet added this to the 0.4.3 milestone Jul 4, 2019
@skylenet skylenet requested a review from janos July 4, 2019 09:48
nonsense
nonsense approved these changes Jul 4, 2019
View reviewed changes
Copy link
Contributor

@nonsense nonsense left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

janos
janos approved these changes Jul 4, 2019
View reviewed changes
Copy link
Member

@janos janos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

@skylenet skylenet merged commit d5f6ee4 into master Jul 4, 2019
@skylenet skylenet self-assigned this Jul 4, 2019
@skylenet skylenet deleted the cmd-bzzaccount-no-require branch July 8, 2019 08:18
vojtechsimetka added a commit that referenced this pull request Jul 9, 2019
@vojtechsimetka
Merge branch 'master' into cheques-protocol
43d3cc3 
* master:
  network/newstream: new stream! protocol base implementation (#1500)
  swarm: fix bzz_info.port when using dynamic port allocation (#1537)
  cmd/swarm: make bzzaccount flag optional and add bzzkeyhex flag (#1531)
  cmd/swarm: remove separate function to parse env vars (#1536)
  network/bitvector: Multibit set/unset + string rep (#1530)
  swarm: 0.4.3 unstable (#1526)
  travis: also build on release tags (#1527)
  swarm: release v0.4.2 (#1496)
  network: bump bzz stream hive (#1522)
  docker: update ca-certificates file (#1525)
  Add swarm guide to /docs (#1513)
  network/simulation: Add ExecAdapter capability to swarm simulations (#1503)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@acud acud acud approved these changes

@janos janos janos approved these changes

@nonsense nonsense nonsense approved these changes

@cobordism cobordism Awaiting requested review from cobordism

Assignees

@skylenet skylenet

Labels
ready for review
Projects
None yet
Milestone
0.4.3
Development

Successfully merging this pull request may close these issues.
5 participants
@skylenet @nonsense @cobordism @janos @acud