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

[GOAL2-624] Add a kmd_config.json.example to the installer dir #112

Merged
merged 4 commits into from Jul 3, 2019

Conversation

EvanJRichard
Copy link
Contributor

Summary

A common request from developers is for some kind of kmd_config.json.example to look at and work from. This PR proposes to add such an example to the installer dir, next to the normal algod config.json.example. It is equivalent to the current default kmd configuration.

Test Plan

Automated test suite will check for regressions.

@EvanJRichard EvanJRichard added documentation Improvements or additions to documentation Enhancement 1 labels Jul 2, 2019
@EvanJRichard EvanJRichard self-assigned this Jul 2, 2019
Copy link
Contributor

@tsachiherman tsachiherman left a comment

Choose a reason for hiding this comment

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

I think that you'll need to do bit more than just that -

  1. Format the json so it would be human readable ( unless there is a good reason not to do so )
  2. Add this json to the following locations -
    • installer/rpm/algorand.spec
    • scripts/build_deb.sh
    • scripts/build_package.sh

@EvanJRichard
Copy link
Contributor Author

@tsachiherman wrote:

I think that you'll need to do bit more than just that -

  1. Format the json so it would be human readable ( unless there is a good reason not to do so )

  2. Add this json to the following locations -

    • installer/rpm/algorand.spec
    • scripts/build_deb.sh
    • scripts/build_package.sh

👍 Formatted the file for 1.), good thinking.
Re 2.) - isn't that only needed if the file is to be installed somewhere? My sole goal here is to provide an example for devs to look at.
Context: To be used, a kmd_config.json needs to be in the kmd-{$version_changes_over_time} data directory, which doesn't exist until kmd is run for the first time. So, I'm not sure it can easily be copied to the right destination from the installer scripts, the desired destination isn't yet created and its version is unknown. (I will find a way if we think it should really be installed, though! 🙂)

Copy link
Contributor

@rotemh rotemh 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 go for another kind of change. Don't create the file yourself. kmd has a function that tries to read a config file, if it fails and returns the default one.
Modify the function to also dump the example file. That way you guarantee consistency of the fields.

if err != nil {
exampleFilename := filepath.Join(dataDir, kmdConfigExampleFilename)
Copy link
Contributor

@Karmastic Karmastic Jul 3, 2019

Choose a reason for hiding this comment

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

Use codecs.SaveObjectToFile() and save it formatted.

Copy link

Choose a reason for hiding this comment

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

Where to modify the default configuration ip and port?

Copy link
Contributor

@tsachiherman tsachiherman left a comment

Choose a reason for hiding this comment

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

Proposed code would save the file on every instance invocation. Was that the intent ?

Copy link
Contributor

@tsachiherman tsachiherman 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. I like the idea of self-generating config files compared to static ones.

@Karmastic Karmastic merged commit 516f64d into algorand:master Jul 3, 2019
algorandskiy pushed a commit to algorandskiy/go-algorand that referenced this pull request Jun 9, 2020
Don't change `assetcreators` table/column names
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation Enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants