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

[discussion] Remove auto generated default-values config #4194

Closed
0x009922 opened this issue Jan 12, 2024 · 7 comments
Closed

[discussion] Remove auto generated default-values config #4194

0x009922 opened this issue Jan 12, 2024 · 7 comments
Assignees
Labels
config-changes Changes in configuration and start up of the Iroha Documentation Documentation changes iroha2-dev The re-implementation of a BFT hyperledger in RUST question Further information is requested

Comments

@0x009922
Copy link
Contributor

What is auto-generated default-values config: those are configs generated by kagami config peer (https://github.com/hyperledger/iroha/blob/15bc2e75dcf6bc0dde9d5109b7ff41f0d753afc5/configs/peer/config.json) and kagami config client (https://github.com/hyperledger/iroha/blob/15bc2e75dcf6bc0dde9d5109b7ff41f0d753afc5/configs/client/config.json). It produces an output with JSON configuration where every single field is specified with its default value. If a field doesn't have a default value and have to be specified by the user, it has null (fields such as iroha/genesis key pair, p2p address etc). Although, the client config also has a key pair hardcoded in kagami (https://github.com/hyperledger/iroha/blob/15bc2e75dcf6bc0dde9d5109b7ff41f0d753afc5/tools/kagami/src/config.rs#L50-L56).

What is the problem I see with that:

  • What is the use case? Is it a tool to help users bootstrap a network? If so, it doesn't help to fill the fields that are really needed to be filled.
  • What for users need to see all default values specified if the values will be applied by Iroha anyway without specification?

My proposal:

  • Remove kagami config subcommand
  • Write configuration samples manually, for Iroha and for Iroha Client CLI. Comment out all lines which have default values, and leave blank spaces for required values. Write tests to ensure that the configuration samples are compatible with Iroha & CLI, and ensure that the default values in the sample are actual.
    • Question: but what if we update the config and default values change

Example of config sample:

[iroha]
public_key = 

[iroha.private_key]
digest = 
payload = 

[genesis]
public_key = 

# specify `genesis.file` and `genesis.private_key` together for the peers
# that submits the genesis
#
# file = 
#
# [genesis.private_key]
# digest = 
# payload = 

[torii]
address = 
# max_content_len = 16_384_000
# query_idle_time = "30s"

# ...

Alternatives:

  • Make kagami config a tool that actually helps to produce a meaningful config. For example:

    kagami config peer --public-key=... --private-key=... --p2p-address=...

Note about compatibility issues: I don't know where to put this note, so it comes here. Some other devs raised a concern that if we provide all parameters set in a config file, although most of them are having their default values, we might introduce problems for the users with compatibility: it increases a chance of configuration incompatibility if we rename/remove some fields and their default values. My note here is that after the configuration overhaul (#2585) and after the 2.0 release, we will have a configuration deprecation and migration policy. We will have a clear process on what guarantees and warnings we give users about configuration parameters deprecations. So, the compatibility issue should not be actual.

@0x009922 0x009922 added question Further information is requested iroha2-dev The re-implementation of a BFT hyperledger in RUST Documentation Documentation changes config-changes Changes in configuration and start up of the Iroha labels Jan 12, 2024
@mversic
Copy link
Contributor

mversic commented Jan 12, 2024

Question: but what if we update the config and default values change

this is the only possible use for this tool. And the generated config should look exactly like your example. Changing defaults does not mean changing the API so we should reserve the right to do so even after 2.0 release. I don't see a reason why we wouldn't want this automated

Make kagami config a tool that actually helps to produce a meaningful config. For example:

we don't need a tool to create a config. Crating config manually is so simple that making a took for that job is silly. Maybe we would want a tool like that to automate the config generation process in scripts, but a simple sed use in bash will accomplish the same thing. I think a tool like this is superfluous atm, if the need arises we can consider it

In the end the most important thing is that the user receives a config that looks exactly like the example that you gave, i.e. with commented out default fields. Whether it's generated manually or automated is of lesser concern for me

@mversic
Copy link
Contributor

mversic commented Jan 12, 2024

I have one problem with the example config that you gave and it's that it's not a valid toml due to unassigned fields. A user can't just take the config file, define env variables and start iroha. They have to first edit the file

@0x009922
Copy link
Contributor Author

I have one problem with the example config that you gave and it's that it's not a valid toml due to unassigned fields. A user can't just take the config file, define env variables and start iroha. They have to first edit the file

Yes, the config sample is the template for users to take it and fill the gaps. Iroha cannot be started without at least essential configuration from the user. I think it would be not good if our config samples have predefined key pairs, for example.

@mversic
Copy link
Contributor

mversic commented Jan 12, 2024

I think it would be not good if our config samples have predefined key pairs, for example

no, this definitely not

I was thinking that keys with missing values should also be commented out. Essentially, everything should be commented out in the config.

@0x009922
Copy link
Contributor Author

I was thinking that keys with missing values should also be commented out. Essentially, everything should be commented out in the config.

The final format of the config sample is a nuance.

What is important now is that it seems that you agree that we can remove kagami config, and hard code config samples once, with comments and default values hints, covering with automated tests.

@mversic
Copy link
Contributor

mversic commented Jan 12, 2024

What is important now is that it seems that you agree that we can remove kagami config, and hard code config samples once, with comments and default values hints, covering with automated tests.

fine by me

@0x009922
Copy link
Contributor Author

0x009922 commented Feb 6, 2024

Closed by #4127. Config sample will be added by #4239.

@0x009922 0x009922 closed this as completed Feb 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
config-changes Changes in configuration and start up of the Iroha Documentation Documentation changes iroha2-dev The re-implementation of a BFT hyperledger in RUST question Further information is requested
Projects
None yet
Development

No branches or pull requests

2 participants