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

Gaiad gentx optional flags #3897

Merged
merged 12 commits into from
Mar 27, 2019
Merged

Gaiad gentx optional flags #3897

merged 12 commits into from
Mar 27, 2019

Conversation

sabau
Copy link
Contributor

@sabau sabau commented Mar 14, 2019

  • add website, details and identity to gentx

  • basic coverage for gentx

  • Closes gaiad gentx should have --website and --detail params #3858

  • Linked to github-issue with discussion and accepted design OR link to spec that describes this work.

  • Wrote tests

  • Updated relevant documentation (docs/)

  • Added a relevant changelog entry: sdkch add [section] [stanza] [message]

  • rereviewed Files changed in the github PR explorer


For Admin Use:

  • Added appropriate labels to PR (ex. wip, ready-for-review, docs)
  • Reviewers Assigned
  • Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)

sabau added 2 commits March 14, 2019 20:16
- start testing it

Signed-off-by: Karoly Albert Szabo <[email protected]>
@codecov
Copy link

codecov bot commented Mar 15, 2019

Codecov Report

❗ No coverage uploaded for pull request base (develop@ea46da7). Click here to learn what that means.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             develop    #3897   +/-   ##
==========================================
  Coverage           ?   60.22%           
==========================================
  Files              ?      196           
  Lines              ?    14551           
  Branches           ?        0           
==========================================
  Hits               ?     8763           
  Misses             ?     5203           
  Partials           ?      585

sabau added 2 commits March 18, 2019 14:12
Signed-off-by: Karoly Albert Szabo <[email protected]>
@sabau sabau changed the title [WIP] Gaiad gentx optional flags Gaiad gentx optional flags Mar 18, 2019
@sabau sabau marked this pull request as ready for review March 18, 2019 13:39
@sabau sabau changed the title Gaiad gentx optional flags [R4R] Gaiad gentx optional flags Mar 18, 2019
@sabau sabau changed the title [R4R] Gaiad gentx optional flags Gaiad gentx optional flags Mar 18, 2019
Copy link
Member

@jackzampolin jackzampolin left a comment

Choose a reason for hiding this comment

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

Pending.md, but otherwise this is a great addition!

Copy link
Contributor

@alexanderbez alexanderbez left a comment

Choose a reason for hiding this comment

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

Thanks @sabau! Left a few bits of feedback, but otherwise LGTM 👍

cmd/gaia/init/gentx.go Outdated Show resolved Hide resolved
cmd/gaia/init/gentx.go Outdated Show resolved Hide resolved
@sabau sabau requested a review from alexanderbez March 26, 2019 16:53
Copy link
Contributor

@alessio alessio left a comment

Choose a reason for hiding this comment

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

I'd like to see the --name flag turning into a positional argument. It'd be out of scope of the issue we're trying to address here though, thus we'll do it in an ensuing PR.
I'm merging this now (dismissing @alexanderbez comments 'cause AFAICS Karoly ha already addressed them all)

@alessio alessio dismissed alexanderbez’s stale review March 27, 2019 18:37

addressed by PR's author

@alessio alessio merged commit 8550d14 into develop Mar 27, 2019
@alessio alessio deleted the sabau/3858-cli-params branch March 27, 2019 18:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants