search

mechboxes / mech

Easy command line virtual machines for VMWare
https://mechboxes.github.io/mech/
MIT License
311 stars 49 forks source link

Multi pr #71

Open mkinney opened 4 years ago

mkinney commented 4 years ago

Major refactoring.

I'd like to continue refactoring/enhancing, but this is a lot of work so far.

Major changes:

ColdHeat commented 4 years ago

This PR is super duper big. I know you just closed some other PRs but it's much easier to process changes broken down by feature instead of all together.

mkinney commented 4 years ago 
mikekinney@sweet mkinney_mech % pytest --cov-report term-missing --cov mech
========================================================= test session starts =========================================================
platform darwin -- Python 3.7.6, pytest-5.3.5, py-1.8.1, pluggy-0.13.1
rootdir: /Users/mikekinney/python/mkinney_mech, inifile: pytest.ini
plugins: mock-2.0.0, cov-2.8.1
collected 79 items

mech/test_mech.py ........................................                                                                      [ 50%]
mech/test_mech_box.py .......                                                                                                   [ 59%]
mech/test_mech_snapshot.py ........                                                                                             [ 69%]
mech/test_utils.py ........................                                                                                     [100%]

---------- coverage: platform darwin, python 3.7.6-final-0 -----------
Name                         Stmts   Miss  Cover   Missing
----------------------------------------------------------
mech/__init__.py                 3      0   100%
mech/__main__.py                16     16     0%   25-46
mech/command.py                 52     30    42%   40, 48, 53, 62-64, 78-82, 89-110, 114
mech/compat.py                  29     10    66%   35-36, 45, 59-66, 71, 76
mech/mech.py                   537    116    78%   497, 540, 565, 656-663, 736, 738, 742, 745, 772, 787, 790-792, 813, 823, 825, 829, 851, 859, 863, 886, 902-904, 911-942, 960, 968, 972, 990, 1029, 1045, 1056-1102, 1124-1126, 1149, 1157, 1168-1202, 1269-1277
mech/test_mech.py              505      0   100%
mech/test_mech_box.py           87      0   100%
mech/test_mech_snapshot.py     146      0   100%
mech/test_utils.py             138      0   100%
mech/utils.py                  451    210    53%   64-67, 72-90, 152-190, 203-216, 253-254, 257, 264, 273, 275, 296, 304-305, 308-309, 311, 315, 317, 331-380, 387, 413, 425, 432, 441, 450, 462-464, 472-473, 477-479, 493-498, 507, 513-514, 518-530, 533-542, 548, 563, 579, 581, 583, 585, 599-600, 603-604, 611-612, 622, 627-628, 639, 644-645, 649-650, 655, 662-663, 668-718
mech/vmrun.py                  240    121    50%   40-47, 55, 60-79, 90-108, 118-121, 160-161, 170, 174-176, 203, 207, 211, 215, 219, 223, 244, 252, 256, 265, 289, 293, 302, 312, 337, 341, 353, 366, 474-476, 488, 498, 502, 506, 514, 518, 529, 541, 545, 549, 553, 557, 561, 570, 579, 583, 587, 591, 595, 599, 603, 607-628, 651, 655, 659, 668, 673, 678, 682, 686, 704, 709, 714, 719, 741, 746, 751, 756, 761, 766
----------------------------------------------------------
TOTAL                         2204    503    77%

========================================================= 79 passed in 4.39s ================================
mkinney commented 4 years ago 
mikekinney@sweet int % pwd
/Users/mikekinney/python/mkinney_mech/tests/int
mikekinney@sweet int % date; ./all; date
Mon Feb 10 17:15:59 PST 2020
 ✓ help
 ✓ no args
 ✓ version

3 tests, 0 failures
 ✓ no Mechfile

1 test, 0 failures
 ✓ mech init, up, destroy of alpine

1 test, 0 failures
 ✓ mech up/destroy of two ubuntu instances (first and second)

1 test, 0 failures
 ✓ mech init, up, destroy of ubuntu from file

1 test, 0 failures
 ✓ provision testing

1 test, 0 failures
 ✓ shared folders testing

1 test, 0 failures
 ✓ add and remove instances tests

1 test, 0 failures
Mon Feb 10 17:26:09 PST 2020
mikekinney@sweet int %
arizvisa commented 4 years ago

(thanks for your work on trying to get this merged. us quiet users appreciate it)

mkinney commented 4 years ago

Thanks. Anything you want to see added? I'm kind of at a good stopping point.

ColdHeat commented 4 years ago

@mkinney you will need to break this up. Tests and bash completions could be separate PRs. As well as could tests.

mkinney commented 4 years ago

There really is no way to break this up. Most of the code has been re-written.

Do you have any suggestions on how that might be done?

ColdHeat commented 4 years ago

https://www.thedroidsonroids.com/blog/splitting-pull-request

You should probably cherry pick the commits that you want into the separate feature branches that they represent.

mkinney commented 4 years ago

There has been very little development or responses on the issues in this repo. I have almost solved all of the issues in this PR.

I understand that it is a HUGE PR. But, there was no responses from my issues/PRs so I just started solving people's issues. I'm working on the last issue now.

The splitting up of the PR is a large task as there was quite a bit of refactoring. It will take quite a bit of work reviewing from people that have commit access to the repo. I am not convinced that will happen.

mkinney commented 4 years ago 
mikekinney@sweet mkinney_mech % pytest -m"int or not int"
================================================== test session starts ===================================================
platform darwin -- Python 3.7.6, pytest-5.3.5, py-1.8.1, pluggy-0.13.1
rootdir: /Users/mikekinney/python/mkinney_mech, inifile: pytest.ini
plugins: mock-2.0.0, xdist-1.31.0, forked-1.1.3, cov-2.8.1
gw0 [126] / gw1 [126] / gw2 [126] / gw3 [126]
.................................................................................................................. [ 90%]
............                                                                                                       [100%]
============================================ 126 passed in 271.26s (0:04:31) ==========
ColdHeat commented 4 years ago

I responded to you and suggested that you break this up over a week ago. You had the opportunity to fix issues piece meal.

Do you expect me to merge this code in and release it without any kind of review?

mkinney commented 4 years ago

How would you review this any differently than broken up?

ColdHeat commented 4 years ago

Here is a simple summary on the benefits of smaller code changes: https://revelry.co/pull-requests-code-review/

mkinney commented 4 years ago

I am familiar with cherry picking and small PRs. I get that smaller changes are easier to review/merge.

However, quite a few core code changes have been made. (ex: removed the whole tracking of the saving of state)

The lack of responses on these PRs and issues should not take 9+ days. The 9+ days response is not just for this PR. The other PRs I had open were much longer. I had to open an issue asking if the repo was active just to get a response. Even that response took 9+ days.

mkinney commented 4 years ago

Perhaps I should request to be a contributor to this repo per a suggestion found here: https://opensource.stackexchange.com/questions/4184/what-should-i-do-if-maintainers-are-unresponsive-how-can-i-become-a-maintainer

mkinney commented 4 years ago

BTW, I do value the feedback. I get that others have really good input on how to implement things. I want to make mech the best tool possible. If there is a better way to do something, I'm willing to discuss and come up with the best approach.

I, too, am frustrated with vagrant. As part of this testing, I went to install/activate my paid for license, but ran into their new licensing thingy... got frustrated and I am going to just use mech going forward.

mkinney commented 4 years ago

So, what is the downside of accepting this PR as is? If there has been no development, then what is the harm?

If there are things that you don't like, feel free to comment on them. I can either remove/change them.

mkinney commented 4 years ago

I've been trying to sort thru what to work on https://github.com/mechboxes/mech/issues/16 .

Perhaps port-forwarding or networking. Not sure which one.

ColdHeat commented 4 years ago

I considered making you a maintainer because you have some good features but you're not setting a great example. @Kronuz was made a maintainer because he made a number of changes and was willing to work with existing maintainers (i.e. me) to get his changes merged in. He emailed me personally and discussed with me his goals and motivations.

If you want to see your changes merged in to the main code base under the main repos, then you need to respect the time of the existing maintainers and respect their wishes. My time is valuable, finite, and better spent than reviewing a ginormous change or accepting features that realistically only affect a small minority.

I recognize that I wasn't responding to your initial messages but I believed @Kronuz would jump in since most of this code was written by him. However I am here now and respecting the time commitment that you're investing in this but you aren't respecting mine. I have some ideas and goals for mech but I no longer use it b/c the world has mostly moved on from virtual machines.

If you want to fork, go ahead and fork. Nothing is stopping you. But if you want to be under the umbrella project, under the pypi package, then you need to work with maintainers. Just as likely, nothing stops me from looking at your fork, re-implemeting your changes under my original design goals and subverting your fork.

mkinney commented 4 years ago

I understand. I really don't want to maintain a fork.

But, look at it from my perspective. I did small three small PRs. I was frustrated. They just sat there. No responses. See https://github.com/mechboxes/mech/pull/64 and https://github.com/mechboxes/mech/pull/66

I added comments to issues: see https://github.com/mechboxes/mech/issues/47 I want to help resolve some/most of these issues. I want to close most of those issues.

I do respect others time. And, I get I have more time to work on things like this. (what I consider fun projects) And, I understand my PRs will take some time reviewing/providing input.

How about re-considering adding me as a maintainer and/or merging this PR. Think about it for a few days. I think with the tests in place, we have a really good foundation for the codebase. (even if stuff is not implemented they way that you like) We can open issues and remove/change them.

But, if you feel like I really am going against the direction or spirit of where you think mech should go, then let me know. I'll start to maintain a fork.

ColdHeat commented 4 years ago

If you're not willing to follow well established good development practices I don't see why I would make you a maintainer.

I offered you insight on your other PRs, you closed them.

You need to break this PR up or it will not be merged in by me.

mkinney commented 4 years ago

I'll be honest, I was offended and hurt by that last comment.

I have added two forms of static code analysis (flake8 and pylint) and added tests (unit and integration). I'm trying to follow good development practices. Some of those pieces can be broken off into separate PRs, but there was quite a bit of re-writing of the classes/functions.

I have only started a long PR because there was little to no activity for the last couple of years on this repo.

I am sorry if I have offended anyone. Not my intent. I'm not the best coder nor communicator, so... it happens. Not intentional. My goal is to create great projects, have happy users, and enjoy doing it in the process.

Are there plans for being more active from the maintainers? If so, great! If not, then what?

mkinney commented 4 years ago

How cool is this: https://codecov.io/gh/mkinney/mech/branch/multi-pr ?!?