CLI Flag --bind

[Bhargav-InfraCloud]

Motivation

Issue 75

Changes

1. Adds --bind flag field in options in main package.
2. Function StartServer (and few other internal functions) made method(s) to a newly introduced type Server.

Summary

• Introduction of new flag --bind for binding-host-address to be read from CLI flag also (earlier: env and default only). The precedence order now is:
  flag --bind=<host> > env HD_BIND=<host> > default value localhost
• StartServer and other internal functions are made methods to type Server in order to limit the number of arguments passed. This way, the code is cleaner.

Related Issues

Closes #75

Message for Maintainers

Given the current setup of function calls in most packages (observed specifically on main, pkg/dashboard and pkg/dashboard/subproc packages with this change), the mocking for unit tests is hardly possible. Hence, the current change was tested manually. Also, the code coverage is low.

Please confirm if issues and PRs will be accepted to revamp code to certain level. Roughly put, this would require the following things:

1. Introduce types in every package and make methods of most functions, if not all.
2. Functionally break/organize methods for testability.
3. Introduce more interfaces for mocking.

[undera]

Very nice, thanks.

[undera]

To your questions @Bhargav-InfraCloud,
I share the opinion that current code structure, quality and test coverage are all quite bad. For example, I'm really dissatisfied with current package/module structure.

Introduce types in every package and make methods of most functions, if not all.

I'm not sure I support the idea of introducing classes/methods for the sake of classes/methods. What's the benefit? I can appreciate the nice class architecture, if it makes sense.

Introduce more interfaces for mocking.

With regard to unit tests, I'd rather mock just the actual functions that call subprocesses, maybe just runCommand(), than all the functions of DataLayer. The problem is that DataLayer changes a lot, it will be a pain to maintain the interface that duplicates DataLayer complexity. In fact, the need to mock only comes to that runCommand() that invokes actual CLI tools.

Functionally break/organize methods for testability.

I'd look at specific examples of what is meant here.

On all of the above - I'd proceed with a separate PR on each of the items, 'cause it's very hard to review the changes that are very massive. In general, I like this direction of better internal architecture and encourage you to continue this conversation in further issues and PRs.

[Bhargav-InfraCloud]

Thanks @undera for the review.
I'll experiment a bit more around the code before dropping the issues/PRs. Just wanted to understand the standpoint of maintainers. You pretty much explained everything I wanted to know 👍

DataLayer changes a lot, it will be a pain to maintain the interface that duplicates DataLayer complexity. In fact, the need to mock only comes to that runCommand() that invokes actual CLI tools.

Ahh! Makes sense. I guess I didn't check all the code or history of it. Noted.

On all of the above - I'd proceed with a separate PR on each of the items, 'cause it's very hard to review the changes that are very massive. In general, I like this direction of better internal architecture and encourage you to continue this conversation in further issues and PRs.

Sure! I like the idea. Will follow. Thanks again.