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

[ioctl] create main for ioctl/newcmd #3296

Merged
merged 41 commits into from
Jun 23, 2022

Conversation

huof6829
Copy link
Contributor

@huof6829 huof6829 commented Apr 13, 2022

Description

Create main function to test ioctl/newcmd/account. Run in command line.
make newioctl
./bin/newioctl account createadd test1

Fixes # (issue)

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

  • Test A
  • Test B

Test Configuration:

  • Firmware version:
  • Hardware:
  • Toolchain:
  • SDK:

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules

@huof6829 huof6829 requested a review from a team as a code owner April 13, 2022 15:30
@codecov
Copy link

codecov bot commented Apr 13, 2022

Codecov Report

Merging #3296 (3f2bb7a) into master (9c24171) will decrease coverage by 0.01%.
The diff coverage is 73.56%.

@@            Coverage Diff             @@
##           master    #3296      +/-   ##
==========================================
- Coverage   75.30%   75.28%   -0.02%     
==========================================
  Files         239      241       +2     
  Lines       22171    22258      +87     
==========================================
+ Hits        16696    16758      +62     
- Misses       4578     4597      +19     
- Partials      897      903       +6     
Impacted Files Coverage Δ
ioctl/newcmd/config/config.go 63.49% <63.49%> (ø)
ioctl/newcmd/account/account.go 89.17% <100.00%> (+0.11%) ⬆️
ioctl/newcmd/root.go 100.00% <100.00%> (ø)
consensus/scheme/rolldpos/rolldposctx.go 73.59% <0.00%> (-0.49%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9c24171...3f2bb7a. Read the comment docs.

@Liuhaai
Copy link
Member

Liuhaai commented Apr 14, 2022

@huof6829
Copy link
Contributor Author

huof6829 commented Apr 14, 2022

done:
make newioctl

@huof6829 huof6829 closed this Apr 14, 2022
@huof6829 huof6829 reopened this Apr 14, 2022
@huof6829 huof6829 removed the pending label Apr 22, 2022
tools/ioctl/newmain/ioctl.go Outdated Show resolved Hide resolved
Makefile Show resolved Hide resolved
@@ -0,0 +1,155 @@
// Copyright (c) 2022 IoTeX Foundation
Copy link
Member

Choose a reason for hiding this comment

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

these 4 new files file another PR like "add config command"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, just add function, but no specific implementation. this will be done after merged

Copy link
Collaborator

Choose a reason for hiding this comment

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

delete

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, delete command function and create new pr for it

@@ -0,0 +1,52 @@
// Copyright (c) 2022 IoTeX Foundation
Copy link
Collaborator

Choose a reason for hiding this comment

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

config_get.go

@@ -0,0 +1,42 @@
// Copyright (c) 2022 IoTeX Foundation
Copy link
Collaborator

Choose a reason for hiding this comment

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

config_reset.go

@@ -0,0 +1,52 @@
// Copyright (c) 2022 IoTeX Foundation
Copy link
Collaborator

Choose a reason for hiding this comment

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

config_set.go

Comment on lines 20 to 21
rootCmd := newcmd.NewIoctl(client)
if err := rootCmd.Execute(); err != nil {
Copy link
Collaborator

Choose a reason for hiding this comment

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

if err := newcmd.NewIoctl(client).Execute(); err != nil {

Comment on lines 20 to 21
rootCmd := newcmd.NewXctl(client)
if err := rootCmd.Execute(); err != nil {
Copy link
Collaborator

Choose a reason for hiding this comment

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

if err := newcmd.NewXctl(client).Execute(); err != nil {

}

// InitConfig load config data from default config file
func InitConfig() (config.Config, string) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

return error

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no need to return err. because if err!=nil, panic.

Copy link
Collaborator

Choose a reason for hiding this comment

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

  1. Multiple places return error
  2. the error handling logic is the same, panic

Thus, returning the error and handle it in the caller side is a better solution

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I know, Thanks.

Comment on lines 64 to 71
if err != nil {
if os.IsNotExist(err) {
err = info.reset() // Config file doesn't exist
}
if err != nil {
log.L().Panic(err.Error())
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

the logic is not clear

)

// Info contains the information of config file
type Info struct {
Copy link
Collaborator

Choose a reason for hiding this comment

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

private struct

@@ -0,0 +1,155 @@
// Copyright (c) 2022 IoTeX Foundation
Copy link
Collaborator

Choose a reason for hiding this comment

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

delete


// Load or reset config file
info.readConfig, err = config.LoadConfig()
if err != nil && os.IsNotExist(err) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

@huof6829 huof6829 merged commit 1f10a0f into iotexproject:master Jun 23, 2022
@huof6829 huof6829 deleted the dev_ioctl_main branch June 23, 2022 03:49
LuckyPigeon pushed a commit to LuckyPigeon/iotex-core that referenced this pull request Jun 23, 2022
CoderZhi added a commit that referenced this pull request Jun 30, 2022
* [ioctl] build hdwallet derive command line into new ioctl

* inline DeriveKey

* adjust order

* revert DeriveKey inline

* create ExportHdwallet client interface

combine hdwalletderive & hdwalletexport code and create ExportHdwallet client interface

* [ioctl] create main for ioctl/newcmd (#3296)

* create main for ioctl/newcmd

* change return type of HdwalletMnemonic

* refactor unit test to cover the modification

* refactor unit test to cover the modification

* fix commit

* fix commit

Co-authored-by: huofei <[email protected]>
Co-authored-by: CoderZhi <[email protected]>
pocockn added a commit to pocockn/iotex-core that referenced this pull request Jul 4, 2022
* upstream/master: (24 commits)
  account type with zero init nonce (iotexproject#3387)
  [api] Separate Server and Server Handler (iotexproject#3485)
  [ioctl] Build hdwallet derive command line into new ioctl (iotexproject#3418)
  [ioctl] Build hdwallet create command line into new ioctl (iotexproject#3470)
  [makefile] add go mod tidy (iotexproject#3471)
  [api] update chain metrics (iotexproject#3484)
  remove config.EVMNetworkID() (iotexproject#3460)
  [filedao] remove checkMasterChainDBFile() (iotexproject#3463)
  [api] add crashlog (iotexproject#3456)
  [api] Move generateBlockMeta to grpcserver.go (iotexproject#3303)
  [ioctl] Build action hash command line into new ioctl (iotexproject#3425)
  [ioctl] Build hdwallet export command line into new ioctl (iotexproject#3423)
  [ioctl] Refactor nodereward command in new ioctl (iotexproject#3416)
  [ioctl] Cleanup TestNewNodeDelegateCmd (iotexproject#3421)
  [blockchain] Remove BoltDBDaoOption (iotexproject#3465)
  remove InMemDaoOption (iotexproject#3464)
  [action] add evm london test (iotexproject#3402)
  [ioctl] create main for ioctl/newcmd (iotexproject#3296)
  [ioctl] Build block bucket command line into new ioctl (iotexproject#3386)
  [ioctl] Build hdwallet import command line into new ioctl (iotexproject#3419)
  ...
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