8000 feat(templates/cmd): upgrade spm to add 'config' cmd by ilgooz · Pull Request #1417 · ignite/cli · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

feat(templates/cmd): upgrade spm to add 'config' cmd #1417

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
Merged
merged 10 commits into from
Aug 3, 2021
Merged

feat(templates/cmd): upgrade spm to add 'config' cmd #1417

merged 10 commits into from
Aug 3, 2021

Conversation

ilgooz
Copy link
Member
@ilgooz ilgooz commented Jul 27, 2021 •
edited by fadeev
Loading

Added support for ~/.marsd/config/client.toml and introduced a way to configure it from config.yml.

Now, instead of having to specify flags (for example, marsd ... --keyring-backend os) every time you run a blockchain's binary, you can set these flags in config.yml:

init:
  client:
    keyring-backend: os

By default, starport chain serve resets the data directory on every file change, but changes from init.client are propagated to client.toml.

To use this feature in an already scaffolded blockchain, upgrade your version of github.com/tendermint/spm in go.mod to v0.1.3.

@ilgooz
feat(templates/cmd): upgrade spm to add 'config' cmd
c006289 
to appd.

* also make $HOME/config/client.toml configurable through 'init.client'
section of chain's Starport config.yml.
@ilgooz ilgooz linked an issue Jul 27, 2021 that may be closed by this pull request
Add an appd config subcommand #1400
Closed
@ilgooz ilgooz marked this pull request as ready for review July 27, 2021 09:09
@ilgooz ilgooz requested review from fadeev, lumtis and Pantani as code owners July 27, 2021 09:09
lumtis
lumtis previously approved these changes Jul 27, 2021
View reviewed changes
Copy link
Contributor
@lumtis lumtis left a comment

Choose a reason for hiding this comment

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

LGTM

@fadeev
Copy link
Contributor
fadeev commented Jul 27, 2021 •
edited
Loading

This is great! The only thing is that by default client.toml contains os as the keyring, but we init everything with test. Hence, you can't use the appd CLI without a keyring backend flag.

@ilgooz
Copy link
Member Author
ilgooz commented Jul 27, 2021

What's your proposition to solve this? Is it using the keyring backend test flag or updating config.toml with test backend? If it is the second option, what if user intentionally wanted to keep os or another backend other than test in the config.toml?

@fadeev
Copy link
Contributor
fadeev commented Jul 27, 2021

Optimal solution is switch everything within serve to use the default keyring (os) and making sure it does not break anything.

@fadeev
Copy link
Contributor
fadeev commented Jul 27, 2021

If using os as default proves to be impractical, having init.client.keyring-backend: test by default in config.yml might be acceptable. Starport uses values from init.client when running commands that need to know which backend needs to be used (except for network commands, which may default to different values).

@ilgooz
Copy link
Member Author
ilgooz commented Jul 27, 2021 •
edited
Loading

Optimal solution is switch everything within serve to use the default keyring (os) and making sure it does not break anything.

As far as I remember, this is not related to serve. Serve already uses the defaults defined in the chain's flagset which is test. And now test isn't used because client.toml has a priority over flag defaults.

To give more info about this: In the past os was default in the chain's flagset and we intentionaly changed it to test in the source code of the chain when we scaffold.

So you mean we should revert our changes in the templates to not overwrite the keyring backend default as test?

In both cases, user might get promped or asked via terminal for a passphrase when you use serve.

@ilgooz
Copy link
Member Author
ilgooz commented Jul 27, 2021 •
edited
Loading

If using os as default proves to be impractical, having init.client.keyring-backend: test by default in config.yml might be acceptable. Starport uses values from init.client when running commands that need to know which backend needs to be used (except for network commands, which may default to different values).

You mean we should scaffold chains with init.client.keyring-backend: test`?

Base automatically changed from hotfix/0.17.1 to master July 27, 2021 13:06
@ilgooz ilgooz changed the base branch from master to develop July 27, 2021 13:13
@ilgooz ilgooz dismissed lumtis’s stale review July 27, 2021 13:13

The base branch was changed.

@fadeev
Copy link
Contributor
fadeev commented Jul 27, 2021

In both cases, user might get promped or asked via terminal for a passphrase when you use serve.

This is not good 🥴

You mean we should scaffold chains with init.client.keyring-backend: test`?

That could be an option, even though it looks like something that shouldn't be in the config file by default.

@ilgooz ilgooz marked this pull request as draft July 28, 2021 19:24
@ilgooz ilgooz marked this pull request as ready for review July 31, 2021 08:37
@ilgooz ilgooz marked this pull request as draft August 1, 2021 11:50
@ilgooz ilgooz marked this pull request as ready for review August 2, 2021 09:27
@fadeev fadeev requested a review from lumtis August 2, 2021 10:03
Pantani
Pantani reviewed Aug 2, 2021
View reviewed changes
@fadeev
Copy link
Contributor
fadeev commented Aug 3, 2021

Changes from config.yml don't get propagated to the client.toml.

Screenshot 2021-08-03 at 10 01 39

@ilgooz
Copy link
Member Author
ilgooz commented Aug 3, 2021

@fadeev solved by 2e73abd.

@ilgooz ilgooz requested a review from Pantani August 3, 2021 06:04
fadeev
fadeev previously approved these changes Aug 3, 2021
View reviewed changes
Copy link
Contributor
@fadeev fadeev left a comment

Choose a reason for hiding this comment

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

Works! 🌊

lumtis
lumtis previously approved these changes Aug 3, 2021
View reviewed changes
Copy link
Contributor
@lumtis lumtis left a comment

Choose a reason for hiding this comment

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

Code looks legit to me outside the lint message

@ilgooz ilgooz dismissed stale reviews from lumtis and fadeev via f2a784a August 3, 2021 08:17
@ilgooz ilgooz merged commit 1eeb307 into develop Aug 3, 2021
@ilgooz ilgooz deleted the feat/tpl-cc branch August 3, 2021 08:26
@fadeev fadeev mentioned this pull request Aug 4, 2021
Closed
@RaulBernal
Copy link
Contributor

Hello :)
It won't be relased for the current main branch?
image

@RaulBernal
Copy link
Contributor

In the meantime, anyway to manually change the produced source-code to compile a binary with the default: os ?

@fadeev
Copy link
Contributor
fadeev commented Aug 9, 2021

@RaulBernal CLI takes its default value from client.toml. In the above example, the CLI would use os backend, because this is what is specified in client.toml.

The (default "test") I think is something that should've been removed. It's a docs issue, doesn't impact the CLI. Is that correct @ilgooz?

@RaulBernal
Copy link
Contributor

Updated spn to 0.1.3 and works!! Thanks @fadeev

ilgooz added a commit that referenced this pull request Aug 10, 2021
@ilgooz @fadeev
feat(templates/cmd): upgrade spm to add 'config' cmd (#1417)
3982708 
* feat(templates/cmd): upgrade spm to add 'config' cmd

to appd.

* also make $HOME/config/client.toml configurable through 'init.client'
section of chain's Starport config.yml.

* default to test

* fix

* fix

* updated changelog

* fix config application order

* fix

Co-authored-by: Denis Fadeev <denis@fadeev.org>
ilgooz added a commit that referenced this pull request Aug 17, 2021
@ilgooz @fadeev
fix(hotfix): v0.17.2 (#1448)
d534217 
* feat(templates/cmd): upgrade spm to add 'config' cmd (#1417)

* feat(templates/cmd): upgrade spm to add 'config' cmd

to appd.

* also make $HOME/config/client.toml configurable through 'init.client'
section of chain's Starport config.yml.

* default to test

* fix

* fix

* updated changelog

* fix config application order

* fix

Co-authored-by: Denis Fadeev <denis@fadeev.org>

* fix(services/chain): detecting keyring (#1449)

* fix(services/chain): detecting keyring

when client.toml > keyring-backend used.

* fix

* fix

* default to test kb

* fix

* fix(services/chain): default home dir for app (#1457)

* fix(services/chain): support 0.43.x based chains (#1455)

* Apply suggestions from code review

* Apply suggestions from code review

* feat(pkg/chaincmd/runner): support YAML encoded CLI response (#1468)

* feat(pkg/chaincmd/runner): support YAML encoded CLI response

* Apply suggestions from code review

* fix

* fix

* fix

* fix

Co-authored-by: Denis Fadeev <denis@fadeev.org>
Jchicode pushed a commit to Jchicode/cli that referenced this pull request Aug 9, 2023
@ilgooz @fadeev
feat(templates/cmd): upgrade spm to add 'config' cmd (ignite#1417)
3f43947 
* feat(templates/cmd): upgrade spm to add 'config' cmd

to appd.

* also make $HOME/config/client.toml configurable through 'init.client'
section of chain's Starport config.yml.

* default to test

* fix

* fix

* updated changelog

* fix config application order

* fix

Co-authored-by: Denis Fadeev <denis@fadeev.org>
Jchicode pushed a commit to Jchicode/cli that referenced this pull request Aug 9, 2023
@ilgooz @fadeev
fix(hotfix): v0.17.2 (ignite#1448)
872f0bd 
* feat(templates/cmd): upgrade spm to add 'config' cmd (ignite#1417)

* feat(templates/cmd): upgrade spm to add 'config' cmd

to appd.

* also make $HOME/config/client.toml configurable through 'init.client'
section of chain's Starport config.yml.

* default to test

* fix

* fix

* updated changelog

* fix config application order

* fix

Co-authored-by: Denis Fadeev <denis@fadeev.org>

* fix(services/chain): detecting keyring (ignite#1449)

* fix(services/chain): detecting keyring

when client.toml > keyring-backend used.

* fix

* fix

* default to test kb

* fix

* fix(services/chain): default home dir for app (ignite#1457)

* fix(services/chain): support 0.43.x based chains (ignite#1455)

* Apply suggestions from code review

* Apply suggestions from code review

* feat(pkg/chaincmd/runner): support YAML encoded CLI response (ignite#1468)

* feat(pkg/chaincmd/runner): support YAML encoded CLI response

* Apply suggestions from code review

* fix

* fix

* fix

* fix

Co-authored-by: Denis Fadeev <denis@fadeev.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fadeev fadeev fadeev left review comments

@lumtis lumtis lumtis left review comments

@Pantani Pantani Awaiting requested review from Pantani

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add an appd config subcommand
5 participants
@ilgooz @fadeev @RaulBernal @Pantani @lumtis
0