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.

Sign up for GitHub

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

Jump to bottom

R4R: Remove advanced gaiacli command #2014

Merged
merged 1 commit into from
Aug 22, 2018
Merged

R4R: Remove advanced gaiacli command #2014

merged 1 commit into from
Aug 22, 2018

Conversation

mslipper
Copy link
Contributor

Closes #1965

@codecov
Copy link

codecov bot commented Aug 14, 2018 •
edited
Loading

Codecov Report

Merging #2014 into develop will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff            @@
##           develop    #2014   +/-   ##
========================================
  Coverage    63.83%   63.83%           
========================================
  Files          113      113           
  Lines         6684     6684           
========================================
  Hits          4267     4267           
  Misses        2133     2133           
  Partials       284      284

melekes
melekes reviewed Aug 14, 2018
View reviewed changes
Gopkg.lock Outdated
@@ -2,76 +2,57 @@


[[projects]]
digest = "1:09a7f74eb6bb3c0f14d8926610c87f569c5cff68e978d30e9a3540aeb626fdf0"
Copy link
Contributor

Choose a reason for hiding this comment

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

please update dep (make update_tools)

This was referenced Aug 14, 2018
Merged
Closed
alexanderbez
alexanderbez suggested changes Aug 14, 2018
View reviewed changes
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.

Looks great! Thanks @mslipper -- I left a tiny remark on the pending changelog and the comment by @melekes should be addressed.

PENDING.md Outdated
@@ -38,6 +38,7 @@ BREAKING CHANGES
* [types] sdk.NewCoin now takes sdk.Int, sdk.NewInt64Coin takes int64
* [cli] #1551: Officially removed `--name` from CLI commands
* [cli] Genesis/key creation (`init`) now supports user-provided key passwords
* [cli] `gaiacli advanced` no longer exists - to access `ibc`, `rest-server`, and `validator-set` commands use `gaiacli ibc`, `gaiacli rest-server`, and `gaiacli tendermint`, respectively
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you reference the PR here? e.g. [cli] #2014 ... or even reference the actual PR? e.g. [cli] [#2014](link) ...

rigelrozanski
rigelrozanski reviewed Aug 14, 2018
View reviewed changes
Copy link
Contributor

@rigelrozanski rigelrozanski left a comment

Choose a reason for hiding this comment

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

looks good besides you need to update your dep

@jackzampolin
Copy link
Member

NICE! This is great!

@jackzampolin
Copy link
Member

@alexanderbez fixed up that small issue and I think this is ready to merge!

@faboweb
Copy link
Contributor

faboweb commented Aug 16, 2018

@cosmos/cosmos-ui breaking

@alexanderbez alexanderbez added C:CLI T: UX ready-for-review T: API Breaking Breaking changes that impact APIs and the SDK only (not state machine). labels Aug 16, 2018
alexanderbez
alexanderbez approved these changes Aug 16, 2018
View reviewed changes
Copy link
Contributor

@alexanderbez alexanderbez left a comment •
edited
Loading

Choose a reason for hiding this comment

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

utACK -- just make sure all the docs & scripts where updated? But I think you covered that.

rigelrozanski
rigelrozanski reviewed Aug 16, 2018
View reviewed changes
PENDING.md Outdated
@@ -39,7 +39,13 @@ BREAKING CHANGES
* [types] sdk.NewCoin now takes sdk.Int, sdk.NewInt64Coin takes int64
* [cli] #1551: Officially removed `--name` from CLI commands
* [cli] Genesis/key creation (`init`) now supports user-provided key passwords
* [cli] [#2014](https://github.com/cosmos/cosmos-sdk/pull/2014) `gaiacli advanced` no longer exists - to access `ibc`, `rest-server`, and `validator-set` commands use `gaiacli ibc`, `gaiacli rest-server`, and `gaiacli tendermint`, respectively
Copy link
Contributor

Choose a reason for hiding this comment

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

For future reference - there is no need to include the github PR link - the issue number/PR should be enough for anyone to look up the PR/issue on github

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry @rigelrozanski I've been doing this as I found it handy as other major projects do it as well.

@rigelrozanski
Copy link
Contributor

why all these CI failing?

@mslipper
Copy link
Contributor Author

make: *** No rule to make target 'install'. Stop.. I'll investigate, see if it's something on my end.

@mslipper
Copy link
Contributor Author

Seems to be a circle CI bug... none of the make tasks exist on CI.

@jackzampolin
Copy link
Member

jackzampolin commented Aug 17, 2018 •
edited
Loading

Looks like the repo isn't getting pulled down into the proper place for some reason. Looks like the repo didn't get cached properly:

No cache is found for key: v1-tree-eceb4635d4cb9e8a3913ac9192dbed9f4e14b00f

cc @greg-szabo any ideas here?

@greg-szabo
Copy link
Member

CircleCI and GitHub status were green at the time... Have you tried turning it off and on again? :trollface:

Also, the setup_dependencies explicitly states that it won't save the cache:

Warning: skipping this step: disabled in configuration

Did anyone change the circleci.yaml?

@greg-szabo
Copy link
Member

Nope, apparently the CircleCI.yaml is fine: https://github.com/mslipper/cosmos-sdk/blob/eceb4635d4cb9e8a3913ac9192dbed9f4e14b00f/.circleci/config.yml#L46

Can anyone with write access see if we disabled caching for outside commits in the CircleCI configuration somewhere? @ebuchman ?

cwgoes
cwgoes previously requested changes Aug 21, 2018
View reviewed changes
Copy link
Contributor

@cwgoes cwgoes left a comment

Choose a reason for hiding this comment

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

Thanks; please rebase against develop and check the CI failures.

@jackzampolin jackzampolin changed the title Remove advanced gaiacli command R4R: Remove advanced gaiacli command Aug 21, 2018
@rigelrozanski rigelrozanski merged commit 62d6fd2 into cosmos:develop Aug 22, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rigelrozanski rigelrozanski rigelrozanski left review comments

@alexanderbez alexanderbez alexanderbez approved these changes

@melekes melekes melekes left review comments

@zramsay zramsay zramsay approved these changes

@cwgoes cwgoes cwgoes left review comments

@ebuchman ebuchman Awaiting requested review from ebuchman

Assignees
No one assigned
Labels
C:CLI T: API Breaking Breaking changes that impact APIs and the SDK only (not state machine). T: UX
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@mslipper @jackzampolin @faboweb @rigelrozanski @greg-szabo @melekes @alexanderbez @zramsay @cwgoes