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

Added note about needing to load daemon. #43

Closed
wants to merge 0 commits into from

Conversation

chrismarino
Copy link
Contributor

Had some problems with my node because I didn't reloading daemon after using the config option. Added a note.

@chrismarino
Copy link
Contributor Author

Sorry about the giant diff in that last commit. I formatted my file and a lot of whitespace changed. Otherwise, just a few lines changed for text and an addition to the header...

@coincashew
Copy link
Owner

coincashew commented Oct 30, 2024

I am unclear what went wrong.

A user should not need to know about manually reloading the daemon. As a general rule, notes are discouraged, as most won't read even if it's spelled out infront of them. Less is more.

It shoudl just work.

Did you change a configuration manually via the terminal and expect it to be live?

Thanks for the PR

@chrismarino
Copy link
Contributor Author

After initial install, I had to I change the config, but said 'No' when asked to restart. Then went back to restart using the other command, which does not reload the config. Seems obvious now, but in the moment I could not figure it out. Spent a lot of time trying to get it all to work. This was a migration from a stand-along CSM validatory, so not a happy path scenario.

@chrismarino
Copy link
Contributor Author

The other changes are just cosmetic. Added some header info on use of CSM plugin, etc.

@coincashew
Copy link
Owner

Understood, so it seems like the best approach would be to always reload after editing, but leave the restart optional. That would have prevented your confusion and headscratching .

Will review tomorrow, cosmetic is always 👍

@chrismarino
Copy link
Contributor Author

chrismarino commented Oct 30, 2024

Yes, that would work. The Title of the submenu is clear (i.e. 'Reload daemon and restart services'), but the message only says 'Do you want to restart validator'? Which is accurate, but without an understanding of the actual steps required to update a service config, how would you know how/when to reload the config after selecting 'No', like I did. Reloading after editing would def fix.

@coincashew
Copy link
Owner

The pull request should be more specific, ideally two PRs, one for cosmetic and another for wording, rather than changing all those whitespaces.

Rather than the wording change, I'd prefer a change in logic -- always reload on edit. Prompt to restart.

I can incorporate this in future changes or feel free to re-submit new PRs.

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.

2 participants