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

[ETCM-491] Use etc.conf as default config file #880

Merged
merged 1 commit into from
Jan 4, 2021

Conversation

enriquerodbe
Copy link
Contributor

Description

Note: This is a continuation of #871. Here I'm addressing the last comment made by @aakoshh in that PR.

If the launcher is executed without arguments, then the default conf/app.conf file is used which uses etc network. This can lead to confusions because a user would expect to make changes to conf/etc.conf and run them using the launcher without arguments.

Proposed Solution

If no argument is received, the launcher will select the conf/etc.conf file as the default configuration. This way the user will see the impacts of changing conf/etc.conf when running the launcher without arguments.

Testing

All validations from #871 should still work. And:

  1. Change conf/etc.conf by setting the network to testnet-internal-nomad
  2. Start the client with bin/mantis-launcher
  3. Verify that the client starts and connects to the testnet-internal-nomad network

Copy link
Contributor

@aakoshh aakoshh left a comment

Choose a reason for hiding this comment

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

Thanks for taking action on my comment, defaulting to the config on empty seems to make sense, otherwise the user is in control and can do whatever they are trying without interference 👍

I took me a while to understand that this behaviour only applies to the mantis-launcher and not mantis itself. It should be possible to inject a whole script file into the mantis script so ./bin/mantis etc would work the same way, but that's a different issue.

@enriquerodbe
Copy link
Contributor Author

Thanks for taking action on my comment, defaulting to the config on empty seems to make sense, otherwise the user is in control and can do whatever they are trying without interference 👍

I took me a while to understand that this behaviour only applies to the mantis-launcher and not mantis itself. It should be possible to inject a whole script file into the mantis script so ./bin/mantis etc would work the same way, but that's a different issue.

Thank you for the quick response 👍

Regarding the launcher: I made that decision mostly because mantis launcher itself works perfectly fine and a user can configure anything by passing the correct parameters. I see the launcher and other bin scripts more as utilities, saving the users from typing -Dconfig.file=conf/$network.conf and letting them just use mantis-launcher $network.

That said, I also found a technical issue trying to extend the mantis script. The issue is that on Windows, the generated mantis.bat uses the arguments array %* directly, so shifting doesn't work, and I couldn't find any other solution to "bypass" a parameter like the shift in bash. So I also preferred consistency across platforms instead of a feature that only works on Unix.

@enriquerodbe enriquerodbe merged commit e9feccb into develop Jan 4, 2021
@enriquerodbe enriquerodbe deleted the feature/ETCM-491-launch-dir branch January 4, 2021 17:02
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